Closed Bug 81011 Opened 19 years ago Closed 19 years ago

_PR_MD_TEST_AND_LOCK has a bad return type

Categories

(NSPR :: NSPR, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mkaply, Assigned: larryh)

Details

Attachments

(2 files)

_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
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
Status: NEW → ASSIGNED
Note that PR_TRUE == 1, PR_FALSE == 0.
PR_SUCCESS == 0, PR_FAILURE == -1.
This is bigger than a breadbox.
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.
Lowered the priority.  _PR_MD_TEST_AND_LOCK is only
used by PR_TestAndLock(), which is a private (though
exported) function.
Priority: P1 → P3
Attached patch Proposed patch.Splinter Review
Keywords: review
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
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.