_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
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
Created attachment 39847 [details] [diff] [review] Proposed patch v2. Use 'PRIntn' instead of 'int'.
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.