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

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[(no longer active)]

Let's finish the awesome work done by mwu.

Comment 3

[(no longer active)]

Attachment: Rewrite script

Comment 4

[(no longer active)]

Attachment: Patch (v1)

I bzipped the patch.  The original patch was 12MB.  This version is 2MB.  Will also push to try.

Comment 5

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=fb3c6d7d472d

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, it would be good to post an additional patch queue fixup script based on mwu's.

Comment 8

[(no longer active)]

Attachment: Patch (v2)

This version of the patch does not touch ipc/chromium.

Comment 9

[(no longer active)]

Attachment: Script for unbitrotting patches

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

Comment 10

[Mozilla RelEng Bot]

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/[e-mail]-fb3c6d7d472d

Comment 11

[Honza Bambas (:mayhemer)]

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

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 13

[Honza Bambas (:mayhemer)]

(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.

Comment 14

[(no longer active)]

(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.

Comment 15

[Honza Bambas (:mayhemer)]

(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

Comment 16

[(no longer active)]

(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?

Comment 17

[Honza Bambas (:mayhemer)]

(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.

Comment 19

[(no longer active)]

Attachment: Rewrite script

This version of the rewrite script excludes ipc/chromium.

Comment 20

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/ec7577dec4fc

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

*.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

[Michael Wu [:mwu]]

(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.