Last Comment Bug 690892 - replace PR_TRUE/PR_FALSE with true/false in mozilla-central
: replace PR_TRUE/PR_FALSE with true/false in mozilla-central
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: :Ehsan Akhgari
:
:
Mentors:
http://groups.google.com/group/mozill...
: 690757 690787 690887 (view as bug list)
Depends on: 675553 711488 809383
Blocks: 695256 731716
  Show dependency treegraph
 
Reported: 2011-09-30 13:11 PDT by :Ehsan Akhgari
Modified: 2012-11-07 03:01 PST (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Rewrite script (726 bytes, text/plain)
2011-09-30 13:17 PDT, :Ehsan Akhgari
no flags Details
Patch (v1) (1.99 MB, application/x-bzip)
2011-09-30 13:27 PDT, :Ehsan Akhgari
dbaron: review+
Details
Patch (v2) (1.98 MB, application/x-bzip)
2011-09-30 14:06 PDT, :Ehsan Akhgari
no flags Details
Script for unbitrotting patches (927 bytes, text/plain)
2011-09-30 14:15 PDT, :Ehsan Akhgari
no flags Details
Rewrite script (765 bytes, text/plain)
2011-10-17 07:56 PDT, :Ehsan Akhgari
no flags Details

Description :Ehsan Akhgari 2011-09-30 13:11:11 PDT
Let's finish the awesome work done by mwu.
Comment 1 :Ehsan Akhgari 2011-09-30 13:11:38 PDT
*** Bug 690887 has been marked as a duplicate of this bug. ***
Comment 2 :Ehsan Akhgari 2011-09-30 13:12:06 PDT
*** Bug 690787 has been marked as a duplicate of this bug. ***
Comment 3 :Ehsan Akhgari 2011-09-30 13:17:56 PDT
Created attachment 563819 [details]
Rewrite script
Comment 4 :Ehsan Akhgari 2011-09-30 13:27:29 PDT
Created attachment 563824 [details]
Patch (v1)

I bzipped the patch.  The original patch was 12MB.  This version is 2MB.  Will also push to try.
Comment 5 :Ehsan Akhgari 2011-09-30 13:34:15 PDT
https://tbpl.mozilla.org/?tree=Try&rev=fb3c6d7d472d
Comment 6 David Baron :dbaron: ⌚️UTC-8 2011-09-30 13:50:06 PDT
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-8 2011-09-30 13:50:41 PDT
Also, it would be good to post an additional patch queue fixup script based on mwu's.
Comment 8 :Ehsan Akhgari 2011-09-30 14:06:54 PDT
Created attachment 563841 [details]
Patch (v2)

This version of the patch does not touch ipc/chromium.
Comment 9 :Ehsan Akhgari 2011-09-30 14:15:13 PDT
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 Mozilla RelEng Bot 2011-09-30 18:30:49 PDT
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
Comment 11 Honza Bambas (:mayhemer) 2011-10-04 08:46:47 PDT
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?) 2011-10-04 11:23:53 PDT
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) 2011-10-04 11:28:01 PDT
(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 :Ehsan Akhgari 2011-10-05 17:03:46 PDT
(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) 2011-10-06 10:39:55 PDT
(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 :Ehsan Akhgari 2011-10-06 11:26:21 PDT
(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) 2011-10-06 11:30:43 PDT
(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 18 :Ms2ger (⌚ UTC+1/+2) 2011-10-16 05:03:32 PDT
*** Bug 690757 has been marked as a duplicate of this bug. ***
Comment 19 :Ehsan Akhgari 2011-10-17 07:56:16 PDT
Created attachment 567444 [details]
Rewrite script

This version of the rewrite script excludes ipc/chromium.
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-10-24 00:56:32 PDT
*.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] 2011-10-24 06:43:34 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.