_PR_MD_TEST_AND_LOCK has a bad return type

RESOLVED FIXED in 4.2

Status

NSPR
NSPR
P3
normal
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: mkaply, Assigned: larryh (gone))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

17 years ago
_PR_MD_TEST_AND_LOCK is defined to return a PRBool, when it should be returning 
a PRStatus.

If you look at:

http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/threads/combined/prulock.
c#417

And

http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/threads/combined/prulock.
c#427

You see that the success case for TEST_AND_LOCK is a return code of 0 which 
equates to PR_SUCCESS. As it stands currently, platforms are return TRUE upon 
failure and false upon success which seems kind of backwards.

Even stranger is that if you look at 

http://lxr.mozilla.org/seamonkey/search?string=TEST_AND_LOCK

some platforms are using PR_SUCCESS and PR_FAILURE for TEST_AND_LOCK

Comment 1

17 years ago
Assigned the bug to Larry.

I agree that _PR_MD_TEST_AND_LOCK should be defined to
return a PRStatus.
Assignee: wtc → larryh
Priority: -- → P1
Target Milestone: --- → 4.2
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

17 years ago
Note that PR_TRUE == 1, PR_FALSE == 0.
PR_SUCCESS == 0, PR_FAILURE == -1.
This is bigger than a breadbox.
(Assignee)

Comment 3

17 years ago
Changing the declaration of PR_TestAndLock() to return PRStatus in 
pr/include/private/pprthred.h is a no-no for 4.2. Suggest leaving the 
declaration as returning PRBool and fixing the implementations to correctly 
return PRBool.

Comment 4

17 years ago
Lowered the priority.  _PR_MD_TEST_AND_LOCK is only
used by PR_TestAndLock(), which is a private (though
exported) function.
Priority: P1 → P3

Comment 5

17 years ago
Created attachment 39788 [details] [diff] [review]
Proposed patch.

Updated

17 years ago
Keywords: review

Comment 6

17 years ago
Created attachment 39847 [details] [diff] [review]
Proposed patch v2.  Use 'PRIntn' instead of 'int'.

Comment 7

17 years ago
I took a stab at this.  As Michael pointed out,
_PR_MD_TEST_AND_LOCK is declared to return a
PRBool but it returns 0 on success and a non-zero
value on failure.  In some places PR_SUCCESS
and PR_FAILURE, the values of the PRStatus type,
are returned.  This is confusing.

So the first thing we should do is to make the
return type of _PR_MD_TEST_AND_LOCK consistent.
Then, there are three possible choices of the
return type.

1. PRIntn.  This is how _PR_MD_TEST_AND_LOCK is
   used in prulock.c, with the convention that
   0 means success and a non-zero value means
   failure, just like the exit status of a Unix
   process.

2. PRBool.  This is how _PR_MD_TEST_AND_LOCK is
   declared in primpl.h.  However, PR_FALSE (0)
   means success and PR_TRUE (in fact any
   non-zero value) means failure, which is
   backwards and confusing.

3. PRStatus.  This is what _PR_MD_TEST_AND_LOCK
   returns on Win16, Win95, and OS/2.

#1 (PRIntn) reflects the current usage.
#2 (PRBool) or #3 (PRStatus) is more descriptive but
would require more changes.  Since PR_TestAndLock()
is a private function, which may even be broken on
some platforms, I decided to go for the least amount
of work.  So I changed _PR_MD_TEST_AND_LOCK to return
a PRIntn, documented the meaning of the return values,
and made it return PRIntn consistently.

I checked in the proposed patch v2 on the trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.