Closed Bug 675553 Opened 9 years ago Closed 9 years ago

Switch from PRBool to bool

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: mwu, Assigned: mwu)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed, Whiteboard: [not-fennec-11])

Attachments

(12 files, 6 obsolete files)

11.00 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
4.75 KB, patch
benjamin
: review+
bholley
: feedback+
Details | Diff | Splinter Review
505 bytes, patch
khuey
: review+
Details | Diff | Splinter Review
1.85 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
8.53 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
49.99 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
1.66 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
1.12 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
5.55 KB, patch
cjones
: review+
Details | Diff | Splinter Review
915 bytes, text/plain
benjamin
: review+
Details
971 bytes, patch
benjamin
: review+
Details | Diff | Splinter Review
1.09 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
Let's switch from PRBool to bool.
Depends on: 675556
Depends on: 675561
Depends on: 675567
Depends on: 676188
Depends on: 676192
Depends on: 676465
Summary: Typedef PRBool to bool → Switch from PRBool to bool
Attached file 1. Commands for s/PRBool/bool/ (obsolete) —
These are the commands I use to generate the patch that converts most of mozilla from PRBool to bool.
Depends on: 676759
Duplicate of this bug: 678133
Blocks: 681188
Depends on: 682547
Attachment #550950 - Attachment is obsolete: true
Attachment #556896 - Flags: review?(benjamin)
Attachment #556898 - Flags: review?(benjamin)
Attachment #556900 - Flags: review?(jones.chris.g)
Attachment #556903 - Flags: review?(khuey)
Making a guess for a reviewer here.
Attachment #556907 - Flags: review?(bzbarsky)
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.
Attachment #556912 - Flags: review?(benjamin)
Comment on attachment 556903 [details] [diff] [review]
4. Make pyidl convert boolean to bool

Yay for neat clean code.
Attachment #556903 - Flags: review?(khuey) → review+
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.
Attachment #556916 - Flags: review?(benjamin)
Attachment #556917 - Flags: review?(benjamin)
Necessary since pldhash.h now requires C++ since it uses bool.
Attachment #556918 - Flags: review?(benjamin)
Attachment #556920 - Flags: review?(benjamin)
Not sure what the real type should be here, but bool isn't it, apparently.
Attachment #556922 - Flags: review?(benjamin)
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 on attachment 556907 [details] [diff] [review]
5. Switch quickstubs to bool

This looks fine.  r=me
Attachment #556907 - Flags: review?(bzbarsky) → review+
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.)
Attachment #556900 - Flags: review?(jones.chris.g)
PRBool/PRPackedBool removed from builtins, some s/PRBool/bool/ redone with better indenting.
Attachment #556900 - Attachment is obsolete: true
Attachment #557165 - Flags: review?(jones.chris.g)
This command does a better job at maintaining indentation.
Attachment #556912 - Attachment is obsolete: true
Attachment #556912 - Flags: review?(benjamin)
Attachment #557241 - Flags: review?(benjamin)
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.
Attachment #557241 - Attachment is obsolete: true
Attachment #557241 - Flags: review?(benjamin)
Attachment #557249 - Flags: review?(benjamin)
Comment on attachment 557165 [details] [diff] [review]
3. Convert prbools in IPDLs to bools, v2

Thanks.
Attachment #557165 - Flags: review?(jones.chris.g) → review+
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..
s///g ?
(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 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.
Attachment #557249 - Flags: review?(benjamin) → review+
Attachment #556896 - Flags: review?(benjamin) → review+
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.
Attachment #556898 - Flags: review?(benjamin)
Attachment #556898 - Flags: review+
Attachment #556898 - Flags: feedback?(bobbyholley+bmo)
Attachment #556914 - Flags: review?(benjamin) → review+
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.
Attachment #556916 - Flags: review?(benjamin) → review-
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?
Attachment #556917 - Flags: review?(benjamin) → review+
Attachment #556918 - Flags: review?(benjamin) → review+
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 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.
Attachment #556922 - Flags: review?(benjamin) → review-
Comment on attachment 556920 [details] [diff] [review]
11. Undo some s/PRBool/bool/ that breaks C code

Tenatively r-, please re-request if necessary.
Attachment #556920 - Flags: review?(benjamin) → review-
(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.
(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..
Attachment #556916 - Flags: review- → review?(benjamin)
Oh, and besides GetBool, there's a similar issue with the Item function.
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.
Attachment #556898 - Flags: feedback?(bobbyholley+bmo) → feedback+
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?
Attachment #556916 - Flags: review?(benjamin) → review+
(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.
I didn't add a !! because the bool type does that for us.
Attachment #556922 - Attachment is obsolete: true
Attachment #560262 - Flags: review?(benjamin)
Attachment #560262 - Flags: review?(benjamin) → review+
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.
Attachment #556920 - Attachment is obsolete: true
Attachment #560284 - Flags: review?(benjamin)
Attachment #560284 - Flags: review?(benjamin) → review+
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
Attachment #557249 - Attachment is patch: true
Attachment #557249 - Attachment is patch: false
https://hg.mozilla.org/mozilla-central/rev/e7854b4d29ba
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
Depends on: 690297
Keywords: dev-doc-needed
Blocks: 690335
Depends on: 690668
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.
Blocks: 690864
Depends on: 690828
No longer blocks: 690864
Depends on: 690864
Blocks: 690857
Blocks: 690892
No longer blocks: 690857
Depends on: 690857
Depends on: 691103
No longer depends on: 691103
Depends on: 692117
Depends on: 692068
Depends on: 691725
Blocks: 693195
Depends on: 693240
Blocks: 695256
Depends on: 695304
Depends on: 696599
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?
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.
Depends on: 720350
Whiteboard: [not-fennec-11]
Blocks: 728148
Depends on: 731917
Duplicate of this bug: 1101715
You need to log in before you can comment on or make changes to this bug.