Closed Bug 690892 Opened 8 years ago Closed 8 years ago

replace PR_TRUE/PR_FALSE with true/false in mozilla-central

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: ehsan, Assigned: ehsan)

References

()

Details

Attachments

(3 files, 2 obsolete files)

Let's finish the awesome work done by mwu.
Duplicate of this bug: 690887
Duplicate of this bug: 690787
Depends on: 675553
Attached file Rewrite script (obsolete) —
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attached file Patch (v1) (obsolete) —
I bzipped the patch.  The original patch was 12MB.  This version is 2MB.  Will also push to try.
Attachment #563824 - Flags: review?(dbaron)
Summary: Remove PR_TRUE/PR_FALSE use in mozila-central → replace PR_TRUE/PR_FALSE with true/false in mozilla-central
Comment on attachment 563824 [details]
Patch (v1)

I think you should also exclude ipc/chromium/ .  r=dbaron with that

In theory, it's correct to use PR_TRUE/PR_FALSE to call NSPR and NSS APIs, but I don't see a good way to leave those and change all the rest.  It shouldn't be harmful since true/false convert to 1/0, which is what PR_TRUE and PR_FALSE are.
Attachment #563824 - Flags: review?(dbaron) → review+
Also, it would be good to post an additional patch queue fixup script based on mwu's.
Attached file Patch (v2)
This version of the patch does not touch ipc/chromium.
Attachment #563824 - Attachment is obsolete: true
This is a script that people can use to unbitrot their patch queues once this lands.
Try run for fb3c6d7d472d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fb3c6d7d472d
Results (out of 230 total builds):
    exception: 2
    success: 218
    warnings: 6
    failure: 4
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-fb3c6d7d472d
The patch includes some changes in the /security module who affect calls to NSS APIs from PSM ; PRBool/PR_TRUE/PR_FALSE usage should be preserved on those places.

Following files need check:
security/manager/ssl/src/nsCMS.cpp
security/manager/ssl/src/nsCertOverrideService.cpp
security/manager/ssl/src/nsDataSignatureVerifier.cpp
security/manager/ssl/src/nsKeyModule.cpp
security/manager/ssl/src/nsNSSCertHelper.cpp
security/manager/ssl/src/nsNSSCertificateDB.cpp
security/manager/ssl/src/nsNSSIOLayer.cpp (nsSSLIOLayerHelpers::rememberTolerantSite)
security/manager/ssl/src/nsNTLMAuthModule.cpp
security/manager/ssl/src/nsPK11TokenDB.cpp
security/manager/ssl/src/nsPKCS11Slot.cpp
security/manager/ssl/src/nsSDR.cpp
security/manager/ssl/src/nsSSLThread.cpp
security/manager/ssl/src/nsSmartCardMonitor.cpp
security/manager/ssl/src/nsStreamCipher.cpp
I agree with dbaron. It is OK to change *all* uses of PR_TRUE/PR_FALSE to true/false in security/manager.

But, do not change anything in security/ that isn't under security/manager. That is, make sure these are excluded:

    security/coreconf
    security/dbm
    security/nss
    security/patches
(In reply to Brian Smith (:bsmith) from comment #12)
> I agree with dbaron. It is OK to change *all* uses of PR_TRUE/PR_FALSE to
> true/false in security/manager.

If it is not causing warnings when calling NSS APIs then OK.
(In reply to Brian Smith (:bsmith) from comment #12)
> I agree with dbaron. It is OK to change *all* uses of PR_TRUE/PR_FALSE to
> true/false in security/manager.
> 
> But, do not change anything in security/ that isn't under security/manager.
> That is, make sure these are excluded:
> 
>     security/coreconf
>     security/dbm
>     security/nss
>     security/patches

I can easily add those 4 directories to my script.
(In reply to Brian Smith (:bsmith) from comment #12)
> I agree with dbaron. It is OK to change *all* uses of PR_TRUE/PR_FALSE to
> true/false in security/manager.
> 
> But, do not change anything in security/ that isn't under security/manager.
> That is, make sure these are excluded:
> 
>     security/coreconf
>     security/dbm
>     security/nss
>     security/patches

This is not what I have said (unless I don't understand your comment correctly).

What I have said is that when calling NSS APIs from code in *security/manager*, we should preserve PR_TRUE/PR_FALSE.  NSS functions expect PRBool arguments, not bool.

An example:
http://hg.mozilla.org/mozilla-central/annotate/552e3737ab7c/security/manager/ssl/src/nsCMS.cpp#l287
(In reply to Honza Bambas (:mayhemer) from comment #15)
> (In reply to Brian Smith (:bsmith) from comment #12)
> > I agree with dbaron. It is OK to change *all* uses of PR_TRUE/PR_FALSE to
> > true/false in security/manager.
> > 
> > But, do not change anything in security/ that isn't under security/manager.
> > That is, make sure these are excluded:
> > 
> >     security/coreconf
> >     security/dbm
> >     security/nss
> >     security/patches
> 
> This is not what I have said (unless I don't understand your comment
> correctly).
> 
> What I have said is that when calling NSS APIs from code in
> *security/manager*, we should preserve PR_TRUE/PR_FALSE.  NSS functions
> expect PRBool arguments, not bool.
> 
> An example:
> http://hg.mozilla.org/mozilla-central/annotate/552e3737ab7c/security/manager/
> ssl/src/nsCMS.cpp#l287

true/false will just be converted to 1/0 by the compiler.  Would that not address your concern?
(In reply to Ehsan Akhgari [:ehsan] from comment #16)
> true/false will just be converted to 1/0 by the compiler.  Would that not
> address your concern?

That was the missing information.  Then it is OK.  Thanks for explanation.
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [To happen Monday, Oct 17 at 8:00AM PST.]
Duplicate of this bug: 690757
Attached file Rewrite script
This version of the rewrite script excludes ipc/chromium.
Attachment #563819 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ec7577dec4fc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [To happen Monday, Oct 17 at 8:00AM PST.]
Blocks: 695256
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #21)
> *.c files in intl and libreg still have PR_TRUE/PR_FALSE...
> 
> http://mxr.mozilla.org/mozilla-central/ident?i=PR_TRUE&filter=
> http://mxr.mozilla.org/mozilla-central/ident?i=PR_FALSE&filter=
> 
> Is this intentional?

Yes. MSVC doesn't support C99 so we can't use bool in C.
Blocks: 731716
Depends on: 809383
You need to log in before you can comment on or make changes to this bug.