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 User image :Ehsan Akhgari 2011-09-30 13:11:11 PDT
Let's finish the awesome work done by mwu.
Comment 1 User image :Ehsan Akhgari 2011-09-30 13:11:38 PDT
*** Bug 690887 has been marked as a duplicate of this bug. ***
Comment 2 User image :Ehsan Akhgari 2011-09-30 13:12:06 PDT
*** Bug 690787 has been marked as a duplicate of this bug. ***
Comment 3 User image :Ehsan Akhgari 2011-09-30 13:17:56 PDT
Created attachment 563819 [details]
Rewrite script
Comment 4 User image :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 User image :Ehsan Akhgari 2011-09-30 13:34:15 PDT
https://tbpl.mozilla.org/?tree=Try&rev=fb3c6d7d472d
Comment 6 User image 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 User image 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 User image :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 User image :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 User image 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 User image 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 User image 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 User image 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 User image :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 User image 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 User image :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 User image 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 User image :Ms2ger (⌚ UTC+1/+2) 2011-10-16 05:03:32 PDT
*** Bug 690757 has been marked as a duplicate of this bug. ***
Comment 19 User image :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 User image Masayuki Nakano [:masayuki] 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 User image 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.