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

RESOLVED FIXED

Status

()

Core
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

Let's finish the awesome work done by mwu.
Duplicate of this bug: 690887
Duplicate of this bug: 690787
Depends on: 675553
Created attachment 563819 [details]
Rewrite script
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Created attachment 563824 [details]
Patch (v1)

I bzipped the patch.  The original patch was 12MB.  This version is 2MB.  Will also push to try.
Attachment #563824 - Flags: review?(dbaron)
https://tbpl.mozilla.org/?tree=Try&rev=fb3c6d7d472d
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.
Created attachment 563841 [details]
Patch (v2)

This version of the patch does not touch ipc/chromium.
Attachment #563824 - Attachment is obsolete: true
Created attachment 563846 [details]
Script for unbitrotting patches

This is a script that people can use to unbitrot their patch queues once this lands.

Comment 10

6 years ago
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.

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [To happen Monday, Oct 17 at 8:00AM PST.]

Updated

6 years ago
Duplicate of this bug: 690757
Created attachment 567444 [details]
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [To happen Monday, Oct 17 at 8:00AM PST.]

Updated

6 years ago
Blocks: 695256
*.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?

Comment 22

6 years ago
(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.
Depends on: 711488
Blocks: 731716
Depends on: 809383
You need to log in before you can comment on or make changes to this bug.