Switch from PRBool to bool

RESOLVED FIXED in mozilla10

Status

()

Core
General
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: mwu, Assigned: mwu)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla10
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [not-fennec-11], URL)

Attachments

(12 attachments, 6 obsolete attachments)

11.00 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
4.75 KB, patch
bsmedberg
: review+
bholley
: feedback+
Details | Diff | Splinter Review
505 bytes, patch
khuey
: review+
Details | Diff | Splinter Review
1.85 KB, patch
Details | Diff | Splinter Review
8.53 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
49.99 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
1.66 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
1.12 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
5.55 KB, patch
cjones
: review+
Details | Diff | Splinter Review
915 bytes, text/plain
bsmedberg
: review+
Details
971 bytes, patch
bsmedberg
: review+
Details | Diff | Splinter Review
1.09 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Let's switch from PRBool to bool.
(Assignee)

Updated

6 years ago
Depends on: 675556
(Assignee)

Updated

6 years ago
Depends on: 675561
(Assignee)

Updated

6 years ago
Depends on: 675567
(Assignee)

Updated

6 years ago
Depends on: 676188
(Assignee)

Updated

6 years ago
Depends on: 676192
(Assignee)

Updated

6 years ago
Depends on: 676465
(Assignee)

Updated

6 years ago
Summary: Typedef PRBool to bool → Switch from PRBool to bool
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Updated

6 years ago
Depends on: 676759

Updated

6 years ago
Duplicate of this bug: 678133
(Assignee)

Updated

6 years ago
Blocks: 681188
(Assignee)

Updated

6 years ago
Depends on: 682547
(Assignee)

Updated

6 years ago
Attachment #550950 - Attachment is obsolete: true
(Assignee)

Comment 3

6 years ago
Created attachment 556896 [details] [diff] [review]
1. Convert C++ code in IDLs to bool
Attachment #556896 - Flags: review?(benjamin)
(Assignee)

Comment 4

6 years ago
Created attachment 556898 [details] [diff] [review]
2. Remove PRBool testing in xpconnect
Attachment #556898 - Flags: review?(benjamin)
(Assignee)

Comment 5

6 years ago
Created attachment 556900 [details] [diff] [review]
3. Convert prbools in IPDLs to bools
Attachment #556900 - Flags: review?(jones.chris.g)
(Assignee)

Comment 6

6 years ago
Created attachment 556903 [details] [diff] [review]
4. Make pyidl convert boolean to bool
Attachment #556903 - Flags: review?(khuey)
(Assignee)

Comment 7

6 years ago
Created attachment 556907 [details] [diff] [review]
5. Switch quickstubs to bool

Making a guess for a reviewer here.
Attachment #556907 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

6 years ago
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.
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+
(Assignee)

Comment 10

6 years ago
Created attachment 556914 [details] [diff] [review]
7. Undo s/PRBool/bool/ in places that interface with NSS
Attachment #556914 - Flags: review?(benjamin)
(Assignee)

Comment 11

6 years ago
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.
Attachment #556916 - Flags: review?(benjamin)
(Assignee)

Comment 12

6 years ago
Created attachment 556917 [details] [diff] [review]
9. Misc fixes for compiler errors
Attachment #556917 - Flags: review?(benjamin)
(Assignee)

Comment 13

6 years ago
Created attachment 556918 [details] [diff] [review]
10. Build bloatblame as C++

Necessary since pldhash.h now requires C++ since it uses bool.
Attachment #556918 - Flags: review?(benjamin)
(Assignee)

Comment 14

6 years ago
Created attachment 556920 [details] [diff] [review]
11. Undo some s/PRBool/bool/ that breaks C code
Attachment #556920 - Flags: review?(benjamin)
(Assignee)

Comment 15

6 years ago
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.
Attachment #556922 - Flags: review?(benjamin)
(Assignee)

Comment 16

6 years ago
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)
(Assignee)

Comment 19

6 years ago
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.
Attachment #556900 - Attachment is obsolete: true
Attachment #557165 - Flags: review?(jones.chris.g)
(Assignee)

Comment 20

6 years ago
Created attachment 557241 [details]
6. Commands to generate a patch that s/PRBool/bool/, v2

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)
(Assignee)

Comment 21

6 years ago
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.
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+
(Assignee)

Comment 23

6 years ago
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 ?
(Assignee)

Comment 25

6 years ago
(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.
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 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-
(Assignee)

Comment 34

6 years ago
(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.
(Assignee)

Comment 35

6 years ago
(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..
(Assignee)

Updated

6 years ago
Attachment #556916 - Flags: review- → review?(benjamin)
(Assignee)

Comment 36

6 years ago
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+
(Assignee)

Comment 39

6 years ago
(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.
(Assignee)

Comment 40

6 years ago
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.
Attachment #556922 - Attachment is obsolete: true
Attachment #560262 - Flags: review?(benjamin)
Attachment #560262 - Flags: review?(benjamin) → review+
(Assignee)

Comment 41

6 years ago
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.
Attachment #556920 - Attachment is obsolete: true
Attachment #560284 - Flags: review?(benjamin)
Attachment #560284 - Flags: review?(benjamin) → review+

Comment 42

6 years ago
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
(Assignee)

Comment 43

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7854b4d29ba
Whiteboard: [inbound]

Updated

6 years ago
Attachment #557249 - Attachment is patch: true
(Assignee)

Updated

6 years ago
Attachment #557249 - Attachment is patch: false
(Assignee)

Comment 44

6 years ago
https://hg.mozilla.org/mozilla-central/rev/e7854b4d29ba
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
Depends on: 690297
(Assignee)

Updated

6 years ago
Keywords: dev-doc-needed

Updated

6 years ago
Blocks: 690335
(Assignee)

Comment 45

6 years ago
Blog post at
https://blog.mozilla.com/mwu/2011/09/29/mozilla-central-now-with-88-7-less-prbool/

https://people.mozilla.com/~mwu/convert.sh

Updated

6 years ago
Depends on: 690668
Depends on: 690669
Depends on: 690700
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.

Updated

6 years ago
Blocks: 690864
Depends on: 690828
No longer blocks: 690864
Depends on: 690864
Blocks: 690857
Blocks: 690892
No longer blocks: 690857
Depends on: 690857
Blocks: 690962
Depends on: 691103
(Assignee)

Updated

6 years ago
No longer depends on: 691103
Depends on: 691124

Updated

6 years ago
Depends on: 692117

Updated

6 years ago
Depends on: 692068
Depends on: 691725

Updated

6 years ago
Blocks: 693195

Updated

6 years ago
Depends on: 693240

Updated

6 years ago
Blocks: 695256

Updated

6 years ago
Depends on: 695304
(Assignee)

Updated

6 years ago
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
Depends on: 842687

Updated

3 years ago
Duplicate of this bug: 1101715
Depends on: 1109972
You need to log in before you can comment on or make changes to this bug.