Last Comment Bug 675553 - Switch from PRBool to bool
: Switch from PRBool to bool
Status: RESOLVED FIXED
[not-fennec-11]
: dev-doc-needed
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Michael Wu [:mwu]
:
Mentors:
https://blog.mozilla.com/mwu/2011/09/...
: 678133 (view as bug list)
Depends on: 669808 675556 675561 675567 676188 676192 676465 676759 682547 690297 690668 690669 690700 690828 690857 690864 691124 691725 692068 692117 693240 695304 696599 720350 731917 842687 1109972
Blocks: 693195 681188 690335 690892 690962 695256 728148
  Show dependency treegraph
 
Reported: 2011-07-31 19:25 PDT by Michael Wu [:mwu]
Modified: 2014-12-10 17:33 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
1. Commands for s/PRBool/bool/ (842 bytes, text/plain)
2011-08-05 00:00 PDT, Michael Wu [:mwu]
no flags Details
1. Convert C++ code in IDLs to bool (11.00 KB, patch)
2011-08-30 10:20 PDT, Michael Wu [:mwu]
benjamin: review+
Details | Diff | Review
2. Remove PRBool testing in xpconnect (4.75 KB, patch)
2011-08-30 10:22 PDT, Michael Wu [:mwu]
benjamin: review+
bobbyholley: feedback+
Details | Diff | Review
3. Convert prbools in IPDLs to bools (5.24 KB, patch)
2011-08-30 10:23 PDT, Michael Wu [:mwu]
no flags Details | Diff | Review
4. Make pyidl convert boolean to bool (505 bytes, patch)
2011-08-30 10:25 PDT, Michael Wu [:mwu]
khuey: review+
Details | Diff | Review
5. Switch quickstubs to bool (1.85 KB, patch)
2011-08-30 10:36 PDT, Michael Wu [:mwu]
bzbarsky: review+
Details | Diff | Review
6. Commands to generate a patch that s/PRBool/bool/ (797 bytes, text/plain)
2011-08-30 10:41 PDT, Michael Wu [:mwu]
no flags Details
7. Undo s/PRBool/bool/ in places that interface with NSS (8.53 KB, patch)
2011-08-30 10:45 PDT, Michael Wu [:mwu]
benjamin: review+
Details | Diff | Review
8. Fixes for calls that are now ambiguous (49.99 KB, patch)
2011-08-30 10:49 PDT, Michael Wu [:mwu]
benjamin: review+
Details | Diff | Review
9. Misc fixes for compiler errors (1.66 KB, patch)
2011-08-30 10:51 PDT, Michael Wu [:mwu]
benjamin: review+
Details | Diff | Review
10. Build bloatblame as C++ (1.12 KB, patch)
2011-08-30 10:53 PDT, Michael Wu [:mwu]
benjamin: review+
Details | Diff | Review
11. Undo some s/PRBool/bool/ that breaks C code (3.84 KB, patch)
2011-08-30 11:01 PDT, Michael Wu [:mwu]
benjamin: review-
Details | Diff | Review
12. Undo a s/PRBool/bool/ that was crashing plugins (543 bytes, patch)
2011-08-30 11:04 PDT, Michael Wu [:mwu]
benjamin: review-
Details | Diff | Review
3. Convert prbools in IPDLs to bools, v2 (5.55 KB, patch)
2011-08-31 07:38 PDT, Michael Wu [:mwu]
cjones.bugs: review+
Details | Diff | Review
6. Commands to generate a patch that s/PRBool/bool/, v2 (850 bytes, text/plain)
2011-08-31 10:53 PDT, Michael Wu [:mwu]
no flags Details
6. Commands to generate a patch that s/PRBool/bool/, v3 (915 bytes, text/plain)
2011-08-31 11:07 PDT, Michael Wu [:mwu]
benjamin: review+
Details
12. Undo a s/PRBool/bool/ that was crashing plugins, v2 (971 bytes, patch)
2011-09-14 15:29 PDT, Michael Wu [:mwu]
benjamin: review+
Details | Diff | Review
11. Make some declarations C++ only (1.09 KB, patch)
2011-09-14 17:00 PDT, Michael Wu [:mwu]
benjamin: review+
Details | Diff | Review

Description Michael Wu [:mwu] 2011-07-31 19:25:10 PDT
Let's switch from PRBool to bool.
Comment 1 Michael Wu [:mwu] 2011-08-05 00:00:00 PDT
Created attachment 550950 [details]
1. Commands for s/PRBool/bool/

These are the commands I use to generate the patch that converts most of mozilla from PRBool to bool.
Comment 2 Ed Morley [:emorley] 2011-08-19 18:26:07 PDT
*** Bug 678133 has been marked as a duplicate of this bug. ***
Comment 3 Michael Wu [:mwu] 2011-08-30 10:20:05 PDT
Created attachment 556896 [details] [diff] [review]
1. Convert C++ code in IDLs to bool
Comment 4 Michael Wu [:mwu] 2011-08-30 10:22:31 PDT
Created attachment 556898 [details] [diff] [review]
2. Remove PRBool testing in xpconnect
Comment 5 Michael Wu [:mwu] 2011-08-30 10:23:59 PDT
Created attachment 556900 [details] [diff] [review]
3. Convert prbools in IPDLs to bools
Comment 6 Michael Wu [:mwu] 2011-08-30 10:25:20 PDT
Created attachment 556903 [details] [diff] [review]
4. Make pyidl convert boolean to bool
Comment 7 Michael Wu [:mwu] 2011-08-30 10:36:22 PDT
Created attachment 556907 [details] [diff] [review]
5. Switch quickstubs to bool

Making a guess for a reviewer here.
Comment 8 Michael Wu [:mwu] 2011-08-30 10:41:55 PDT
Created attachment 556912 [details]
6. Commands to generate a patch that s/PRBool/bool/

These are the commands I use to generate a patch and then modify the patch to s/PR_TRUE/true/ s/PR_FALSE/false/ on modified lines in the patch.

Actual patch not attached because it's about 8.5mb and very quickly bitrots.
Comment 9 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-30 10:43:39 PDT
Comment on attachment 556903 [details] [diff] [review]
4. Make pyidl convert boolean to bool

Yay for neat clean code.
Comment 10 Michael Wu [:mwu] 2011-08-30 10:45:22 PDT
Created attachment 556914 [details] [diff] [review]
7. Undo s/PRBool/bool/ in places that interface with NSS
Comment 11 Michael Wu [:mwu] 2011-08-30 10:49:09 PDT
Created attachment 556916 [details] [diff] [review]
8. Fixes for calls that are now ambiguous

Certain calls, SetBool especially, confuse the compiler if PR_TRUE/FALSE is used instead of true/false since the compiler can't decide which function we mean. This patch fixes those cases by doing s/PR_FALSE/false/ as necessary.
Comment 12 Michael Wu [:mwu] 2011-08-30 10:51:01 PDT
Created attachment 556917 [details] [diff] [review]
9. Misc fixes for compiler errors
Comment 13 Michael Wu [:mwu] 2011-08-30 10:53:57 PDT
Created attachment 556918 [details] [diff] [review]
10. Build bloatblame as C++

Necessary since pldhash.h now requires C++ since it uses bool.
Comment 14 Michael Wu [:mwu] 2011-08-30 11:01:26 PDT
Created attachment 556920 [details] [diff] [review]
11. Undo some s/PRBool/bool/ that breaks C code
Comment 15 Michael Wu [:mwu] 2011-08-30 11:04:13 PDT
Created attachment 556922 [details] [diff] [review]
12. Undo a s/PRBool/bool/ that was crashing plugins

Not sure what the real type should be here, but bool isn't it, apparently.
Comment 16 Michael Wu [:mwu] 2011-08-30 11:10:18 PDT
And that's all the patches I have for now.

Basically, the strategy is to do a massive search and replace on the whole tree and undo the parts that aren't appropriate. That's patches 6-12 here.

All patches need to be applied for this to work. Patches which can land separately have their own bugs.

Not sure what people want to do in terms of closing trees/scheduling for this.
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-08-30 14:11:05 PDT
Comment on attachment 556907 [details] [diff] [review]
5. Switch quickstubs to bool

This looks fine.  r=me
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-30 23:03:19 PDT
Comment on attachment 556900 [details] [diff] [review]
3. Convert prbools in IPDLs to bools

Would you please also nuke these guys http://mxr.mozilla.org/mozilla-central/source/ipc/ipdl/ipdl/builtin.py#64 so we don't need to worry about this again?  (Or at least, worry about it less.)
Comment 19 Michael Wu [:mwu] 2011-08-31 07:38:00 PDT
Created attachment 557165 [details] [diff] [review]
3. Convert prbools in IPDLs to bools, v2

PRBool/PRPackedBool removed from builtins, some s/PRBool/bool/ redone with better indenting.
Comment 20 Michael Wu [:mwu] 2011-08-31 10:53:35 PDT
Created attachment 557241 [details]
6. Commands to generate a patch that s/PRBool/bool/, v2

This command does a better job at maintaining indentation.
Comment 21 Michael Wu [:mwu] 2011-08-31 11:07:09 PDT
Created attachment 557249 [details]
6. Commands to generate a patch that s/PRBool/bool/, v3

And this one does an even better job of maintaining indentation.

It's not perfect, but I think it covers most cases sufficiently that we can live with the cases that it misses.
Comment 22 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-31 12:21:39 PDT
Comment on attachment 557165 [details] [diff] [review]
3. Convert prbools in IPDLs to bools, v2

Thanks.
Comment 23 Michael Wu [:mwu] 2011-09-06 10:38:18 PDT
Just discovered that the sed command for replacing PR_TRUE/PR_FALSE should actually be run about three times to cover cases where there is more than one PR_TRUE/PR_FALSE on the same line. There's probably a smarter sed command to do this..
Comment 24 Masatoshi Kimura [:emk] 2011-09-06 10:55:12 PDT
s///g ?
Comment 25 Michael Wu [:mwu] 2011-09-06 10:59:06 PDT
(In reply to Masatoshi Kimura [:emk] from comment #24)
> s///g ?

Nope. The current regex is too greedy for s///g to make a difference.
Comment 26 Ed Morley [:emorley] 2011-09-06 11:03:31 PDT
Something like:

    -e ':loop
        s/foo/bar/g
        t loop' \

(http://www.thegeekstuff.com/2009/12/unix-sed-tutorial-6-examples-for-sed-branching-operation/)
Comment 27 Benjamin Smedberg [:bsmedberg] 2011-09-14 09:28:58 PDT
Comment on attachment 557249 [details]
6. Commands to generate a patch that s/PRBool/bool/, v3

Blech, I think I'd prefer emacs-lisp to actually do reindentation, but I guess that this worse-is-better approach is reasonable.
Comment 28 Benjamin Smedberg [:bsmedberg] 2011-09-14 09:36:00 PDT
Comment on attachment 556898 [details] [diff] [review]
2. Remove PRBool testing in xpconnect

I think this is fine, but bholley may have patches which conflict, we should ask him.
Comment 29 Benjamin Smedberg [:bsmedberg] 2011-09-14 09:42:12 PDT
Comment on attachment 556916 [details] [diff] [review]
8. Fixes for calls that are now ambiguous

Why are all the changes to Preferences::GetBool necessary? Doesn't the other patch automatically replace PR_FALSE with false? I'm not opposed to this, but I don't understand it.
Comment 30 Benjamin Smedberg [:bsmedberg] 2011-09-14 09:43:47 PDT
Comment on attachment 556917 [details] [diff] [review]
9. Misc fixes for compiler errors

This is safe because we blow away the startup cache every time the buildid or version changes, right?
Comment 31 Benjamin Smedberg [:bsmedberg] 2011-09-14 09:47:08 PDT
Comment on attachment 556920 [details] [diff] [review]
11. Undo some s/PRBool/bool/ that breaks C code

What C code do we have left? Could we just stop supporting C code here?
Comment 32 Benjamin Smedberg [:bsmedberg] 2011-09-14 09:51:59 PDT
Comment on attachment 556922 [details] [diff] [review]
12. Undo a s/PRBool/bool/ that was crashing plugins

Make this a PRUint32 please and then use !! to set *wantsAllStreams? This will also be necessary in AnswerNPP_GetValue_NPPVpluginNeedsXEmbed.
Comment 33 Benjamin Smedberg [:bsmedberg] 2011-09-14 09:52:34 PDT
Comment on attachment 556920 [details] [diff] [review]
11. Undo some s/PRBool/bool/ that breaks C code

Tenatively r-, please re-request if necessary.
Comment 34 Michael Wu [:mwu] 2011-09-14 10:04:07 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #29)
> Comment on attachment 556916 [details] [diff] [review]
> 8. Fixes for calls that are now ambiguous
> 
> Why are all the changes to Preferences::GetBool necessary? Doesn't the other
> patch automatically replace PR_FALSE with false? I'm not opposed to this,
> but I don't understand it.

The script that replaces PR_FALSE with false only does it on the lines that we've done a s/PRBool/bool/ replacement on, in order to reduce the number of changes.

The reason this is needed is because there are two Preferences::GetBool:

static PRBool GetBool(const char* aPref, PRBool aDefault = PR_FALSE)
static nsresult GetBool(const char* aPref, PRBool* aResult)

The compiler doesn't know which one we want if we don't use false.
Comment 35 Michael Wu [:mwu] 2011-09-14 10:07:58 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #30)
> Comment on attachment 556917 [details] [diff] [review]
> 9. Misc fixes for compiler errors
> 
> This is safe because we blow away the startup cache every time the buildid
> or version changes, right?

Yeah the startup cache should be blown away on updates. Though, it might even end up the same since we haven't changed the size of the type..
Comment 36 Michael Wu [:mwu] 2011-09-14 10:14:00 PDT
Oh, and besides GetBool, there's a similar issue with the Item function.
Comment 37 Bobby Holley (busy) 2011-09-14 10:16:29 PDT
Comment on attachment 556898 [details] [diff] [review]
2. Remove PRBool testing in xpconnect

These files are moving from tests/components to tests/components/native, but otherwise they're fine. It's a trivial merge, so whichever one of us lands second can take care of it.
Comment 38 Benjamin Smedberg [:bsmedberg] 2011-09-14 11:23:48 PDT
Comment on attachment 556916 [details] [diff] [review]
8. Fixes for calls that are now ambiguous

Do you have plans to replace all the PR_TRUE, or is that just considered too invasive for now?
Comment 39 Michael Wu [:mwu] 2011-09-14 12:01:48 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #38)
> Comment on attachment 556916 [details] [diff] [review]
> 8. Fixes for calls that are now ambiguous
> 
> Do you have plans to replace all the PR_TRUE, or is that just considered too
> invasive for now?

I was considering doing a whole tree s/PR_TRUE/true/ s/PR_FALSE/false/ after a few months to allow it to naturally happen as people land their patches. I'm assuming that most people would kill PR_TRUE/PR_FALSE in the code they're touching if given a choice. But, we can do it sooner if that's better.
Comment 40 Michael Wu [:mwu] 2011-09-14 15:29:09 PDT
Created attachment 560262 [details] [diff] [review]
12. Undo a s/PRBool/bool/ that was crashing plugins, v2

I didn't add a !! because the bool type does that for us.
Comment 41 Michael Wu [:mwu] 2011-09-14 17:00:48 PDT
Created attachment 560284 [details] [diff] [review]
11. Make some declarations C++ only

Apparently none of the C code use the things that are C++ only now, so we can just take these things away in that case.
Comment 42 Mozilla RelEng Bot 2011-09-27 20:11:01 PDT
Try run for c7a807408637 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c7a807408637
Results (out of 231 total builds):
    exception: 1
    success: 218
    warnings: 10
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mwu@mozilla.com-c7a807408637
Comment 44 Michael Wu [:mwu] 2011-09-29 01:40:23 PDT
https://hg.mozilla.org/mozilla-central/rev/e7854b4d29ba
Comment 46 Bill Gianopoulos [:WG9s] 2011-09-30 08:31:41 PDT
Just an observation.  There seems to be an issue with the way this is being rolled out.  It seems to me that once this is completed for an area of the code, then PRBool PR_TRUE and PR_FALSE need to be redefined in a way that causes a compiler error.  The issue stems from pending already R+'d patches that add more uses of PRBool, PR_TRUE and PR_FALSE.
Comment 47 Bill Gianopoulos [:WG9s] 2011-10-28 09:24:58 PDT
I think we need to remove the definitions for PRBool PR_TRUE and PR_FALSE from any global include files and make sure these are only defined in modules where their use is still p=permitted.

The issue is there are many unlanded r+'d patches that add new instances of this.

Should I file a new bug on this?
Comment 48 Eric Shepherd [:sheppy] 2012-01-04 06:55:40 PST
Given the enormity of this change to the documentation, here's how we're handling it (for the record):

1. We've noted this change on Firefox 10 for developers, as well as on the article about updating add-ons for Firefox 10.

2. We are not actively updating any other documentation at this time.

3. Once we're on the new Kuma wiki for MDN, and it has its global find and replace feature implemented, we'll begin work on actually actively updating documentation for this change everywhere else.

So, for my purposes, this is done for Firefox 10, but leaving the doc needed flag on it to remember to do (3) later.
Comment 49 Josh Matthews [:jdm] 2014-11-19 11:42:54 PST
*** Bug 1101715 has been marked as a duplicate of this bug. ***

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