81011 - _PR_MD_TEST_AND_LOCK has a bad return type

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P3)

Description

[Mike Kaply [:mkaply]]

_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

[Wan-Teh Chang]

Assigned the bug to Larry.

I agree that _PR_MD_TEST_AND_LOCK should be defined to
return a PRStatus.

Comment 2

[larryh (gone)]

Note that PR_TRUE == 1, PR_FALSE == 0.
PR_SUCCESS == 0, PR_FAILURE == -1.
This is bigger than a breadbox.

Comment 3

[larryh (gone)]

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

[Wan-Teh Chang]

Lowered the priority.  _PR_MD_TEST_AND_LOCK is only
used by PR_TestAndLock(), which is a private (though
exported) function.

Comment 5

[Wan-Teh Chang]

Attachment: Proposed patch.

Comment 6

[Wan-Teh Chang]

Attachment: Proposed patch v2. Use 'PRIntn' instead of 'int'.

Comment 7

[Wan-Teh Chang]

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.