Closed Bug 228448 Opened 16 years ago Closed 5 months ago

Remove '=='/'!=' 'PR_FALSE'/'PR_TRUE'

Categories

(Core :: General, task, trivial)

task
Not set
trivial

Tracking

()

RESOLVED FIXED

People

(Reporter: sgautherie, Assigned: sgautherie)

References

(Depends on 1 open bug, )

Details

(Keywords: helpwanted, Whiteboard: [good first bug][not-ready-for-cedar])

Attachments

(7 files, 6 obsolete files)

24.38 KB, patch
neil
: review+
Details | Diff | Splinter Review
9.02 KB, patch
sgautherie
: review+
mscott
: superreview+
Details | Diff | Splinter Review
7.57 KB, patch
kaie
: review+
Details | Diff | Splinter Review
14.37 KB, patch
Bienvenu
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
1.05 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
7.91 KB, patch
dean_tessman
: review+
benjamin
: approval2.0-
Details | Diff | Splinter Review
10.19 KB, patch
Biesinger
: review+
benjamin
: approval2.0-
Details | Diff | Splinter Review
Enforce:
<http://www.mozilla.org/hacking/mozilla-style-guide.html>
{
Do not compare == PR_TRUE or PR_FALSE. Use (x) or (!x) instead. == PR_TRUE, in
fact, is *different* from if (x)!
}

Need a fix:
<http://lxr.mozilla.org/seamonkey/search?string=%5C%3D%5C%3D%5C+PR_TRUE>
<http://lxr.mozilla.org/seamonkey/search?string=%5C%3D%5C%3D%5C+PR_FALSE>
Status: NEW → ASSIGNED
Need a fix too:
<http://lxr.mozilla.org/seamonkey/search?string=%5C%21%5C%3D%5C+PR_FALSE>
<http://lxr.mozilla.org/seamonkey/search?string=%5C%21%5C%3D%5C+PR_TRUE>
Summary: Remove '== PR_FALSE' and '== PR_TRUE' → Remove '=='/'!=' 'PR_FALSE'/'PR_TRUE'
Also:
*Removes unneeded brackets of '{ // 1 line }' blocks.
*Separates 'if( (rv = xyz) )' into 'rv = xyz; if(rv)'.
*Does 2 ' ? : ' code rewrites.
Comment on attachment 137404 [details] [diff] [review]
(Av1) <nsInternetSearchService.cpp>


'r=?': (see comment 2)
Can you (super-)review, compile, test, check it in ?
Attachment #137404 - Flags: superreview?(alecf)
Attachment #137404 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 137404 [details] [diff] [review]
(Av1) <nsInternetSearchService.cpp>

reviewers do not compile and test. Don't ask for a review until you've done
that much yourself.
Attachment #137404 - Flags: superreview?(alecf)
Comment on attachment 137404 [details] [diff] [review]
(Av1) <nsInternetSearchService.cpp>

Please don't change the brace style, since nobody can agree on what it should
be (Mozilla permits the use of four different brace styles).
Attachment #137404 - Flags: review?(neil.parkwaycc.co.uk) → review-
Av1, with comment 5 suggestions.
Attachment #137404 - Attachment is obsolete: true
Comment on attachment 137796 [details] [diff] [review]
(Av2) <nsInternetSearchService.cpp>
[Checked in: Comment 11]


I have no compiler: Could you compile/test/review it ? Thanks.
Attachment #137796 - Flags: review?(neil.parkwaycc.co.uk)
Target Milestone: --- → mozilla1.7alpha
Attachment #137796 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #137796 - Flags: superreview?(alecf)
I will not review patches until they have been compiled and tested. Let me know
when that happens.
Comment on attachment 137796 [details] [diff] [review]
(Av2) <nsInternetSearchService.cpp>
[Checked in: Comment 11]

I've used Mozilla with this patch applied although I've not specifically
regression tested this module.
Comment on attachment 137796 [details] [diff] [review]
(Av2) <nsInternetSearchService.cpp>
[Checked in: Comment 11]

sr=alecf then.
Attachment #137796 - Flags: superreview?(alecf) → superreview+
Comment on attachment 137796 [details] [diff] [review]
(Av2) <nsInternetSearchService.cpp>
[Checked in: Comment 11]


Check in: { 12/29/2003 03:18	neil%parkwaycc.co.uk	1.200 }
Attachment #137796 - Attachment description: (Av2) <nsInternetSearchService.cpp> → (Av2) <nsInternetSearchService.cpp> [Checked in: Comment 11]
Attachment #137796 - Attachment is obsolete: true
Comment on attachment 137796 [details] [diff] [review]
(Av2) <nsInternetSearchService.cpp>
[Checked in: Comment 11]


If there was no hidden work,
then this fix reduced the footprint too ;-)
{ Zdiff:-48 (+86/-134) }
Comment on attachment 138132 [details] [diff] [review]
(Bv1) </mailnews/addrbook/src/ns*.*>


I have no compiler: Could you compile/test/review it ? Thanks.
Attachment #138132 - Flags: review?(neil.parkwaycc.co.uk)
Depends on: 230001
Comment on attachment 138132 [details] [diff] [review]
(Bv1) </mailnews/addrbook/src/ns*.*>

Just a few nits:

>-        if (operation == nsIAbBooleanOperationTypes::OR &&
>-            value == PR_TRUE)
>-            break;
>-        else if (operation == nsIAbBooleanOperationTypes::AND &&
>-            value == PR_FALSE)
>+        if ((operation == nsIAbBooleanOperationTypes::OR && value) ||
>+            (operation == nsIAbBooleanOperationTypes::AND && !value))
I think merging this makes it less readable.

>             break;
>         else if (operation == nsIAbBooleanOperationTypes::NOT)
>-            value = (value == PR_TRUE) ? PR_FALSE : PR_TRUE;
>+            value = value ? PR_FALSE : PR_TRUE;
value = !value;

>-    if(mInitialized == PR_TRUE)
>+    if(mInitialized)
Might as well put a space between if and (
Attachment #138132 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #138132 - Attachment is obsolete: true
Comment on attachment 139060 [details] [diff] [review]
(Bv1b) </mailnews/addrbook/src/ns*.*>
[Checked in: Comment 18]


{ (Bv1) </mailnews/addrbook/src/ns*.*>		 patch		2003-12-29
15:10:42     neil.parkwaycc.co.uk: review+  }
Attachment #139060 - Flags: superreview?(sspitzer)
Attachment #139060 - Flags: review+
Attachment #139060 - Flags: superreview?(sspitzer) → superreview?(mscott)
Target Milestone: mozilla1.7alpha → ---
Attachment #139060 - Flags: superreview?(mscott) → superreview+
Comment on attachment 139060 [details] [diff] [review]
(Bv1b) </mailnews/addrbook/src/ns*.*>
[Checked in: Comment 18]


Check in: { 2004-04-22 16:13	neil%parkwaycc.co.uk }
Attachment #139060 - Attachment description: (Bv1b) </mailnews/addrbook/src/ns*.*> → (Bv1b) </mailnews/addrbook/src/ns*.*> [Checked in: Comment 18]
Attachment #139060 - Attachment is obsolete: true
Product: Browser → Seamonkey
Attachment #139060 - Attachment is obsolete: false
Attachment #137796 - Attachment is obsolete: false
Flags: in-testsuite-
Product: SeaMonkey → Core
QA Contact: general → general
Attached patch (Cv1) <nsBookmarksService.cpp> (obsolete) — Splinter Review
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080809194348 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)

Compiled, not specifically tested.
Attachment #333119 - Flags: review?(db48x)
Attached patch (Dv1) <nsNSSComponent.cpp> (obsolete) — Splinter Review
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre)
Gecko/20080809194348 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)

Compiled, not specifically tested.
Attachment #333125 - Flags: review?(kaie)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre)
Gecko/20080809194348 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)

Compiled, not specifically tested.
Attachment #333125 - Attachment is obsolete: true
Attachment #333133 - Flags: review?(kaie)
Attachment #333125 - Flags: review?(kaie)
Attachment #333119 - Flags: review?(db48x)
Comment on attachment 333133 [details] [diff] [review]
(Dv1a) </security/manager/ssl/src/ns*.cpp>
[Checkin: Comment 23]

r=kaie, thanks!
Attachment #333133 - Flags: review?(kaie) → review+
Keywords: checkin-needed
Whiteboard: [c-n: Dv1a // Leave opened]
Dv1a pushed in 17116:0380e2e6598a.
Keywords: checkin-needed
Whiteboard: [c-n: Dv1a // Leave opened]
Attachment #333133 - Attachment description: (Dv1a) </security/manager/ssl/src/ns*.cpp> → (Dv1a) </security/manager/ssl/src/ns*.cpp> [Checkin: Comment 23]
Cv1, unbitrotted.

Daniel, in 2008.08, you emailed me:
"I actually meant to reassign it to someone else. Guess I messed it
up."
Let's restart from there...
Attachment #333119 - Attachment is obsolete: true
Attachment #371831 - Flags: review?(db48x)
Keywords: helpwanted
Whiteboard: [good first bug]
Attachment #371843 - Flags: superreview?(bienvenu)
Attachment #371843 - Flags: review?(bienvenu)
Depends on: 487605
Comment on attachment 371843 [details] [diff] [review]
(Ev1-CC) </mailnews/*> except 1
[Checkin: Comment 27]

thx for the patch.
Attachment #371843 - Flags: superreview?(bienvenu)
Attachment #371843 - Flags: superreview+
Attachment #371843 - Flags: review?(bienvenu)
Attachment #371843 - Flags: review+
Comment on attachment 371843 [details] [diff] [review]
(Ev1-CC) </mailnews/*> except 1
[Checkin: Comment 27]


http://hg.mozilla.org/comm-central/rev/9c76df358a51
Attachment #371843 - Attachment description: (Ev1-CC) </mailnews/*> except 1 → (Ev1-CC) </mailnews/*> except 1 [Checkin: Comment 27]
Depends on: 517553
Sooo, this should be marked FIXED?
(In reply to comment #28)
> Sooo, this should be marked FIXED?

Nooo, this remains to BE fixed!
Current status is "Found 307 matching lines in 112 files".
Attachment #486089 - Flags: review?(dean_tessman)
Attachment #371831 - Attachment description: (Cv1a-SM) <nsBookmarksService.cpp> → (Cv1a-SM) <nsBookmarksService.cpp> [Obsoleted by bug 580663]
Attachment #371831 - Attachment is obsolete: true
Attachment #371831 - Flags: review?(db48x)
Comment on attachment 486089 [details] [diff] [review]
(Gv1) /xpfe/components/directory/nsDirectoryViewer.cpp

It's not quite the scope of this but, but I see you're removing setting a value to the PRBool in cases where it gets a value in a function call on the next line.  You might as well take care of this, too, then:

  PRBool isContainerFlag = PR_FALSE;

  if (node && NS_SUCCEEDED(node->EqualsNode(kTrueLiteral, &isContainerFlag)))
Attachment #486089 - Flags: review?(dean_tessman) → review-
Attachment #486084 - Flags: review?(bienvenu) → review+
Comment on attachment 486084 [details] [diff] [review]
(Fv1-CC) /mailnews/compose/src/nsMsgCompose.cpp
[Checked in: Comment 34]

http://hg.mozilla.org/comm-central/rev/a124a417bc88
Attachment #486084 - Attachment description: (Fv1) /mailnews/compose/src/nsMsgCompose.cpp → (Fv1-CC) /mailnews/compose/src/nsMsgCompose.cpp [Checked in: Comment 34]
(In reply to comment #34)
> Comment on attachment 486084 [details] [diff] [review]
> (Fv1-CC) /mailnews/compose/src/nsMsgCompose.cpp
> [Checked in: Comment 34]
> 
> http://hg.mozilla.org/comm-central/rev/a124a417bc88

By the way:

""If you have a patch to land, you need to seek approval from me, either direct via email or over irc. You'll then need to include 'CLOSED TREE' in your checkin comment. The basic criteria will be patches unlikely to affect Mac xpcshell-tests areas (e.g. UI patches, some build config patches, possibly some back-end patches).""-Standard8

Though since this is comment-only, I'm sticking a retro-active a=Callek on that patch.
(In reply to comment #35)
> ""If you have a patch to land, you need to seek approval from me

Uh. Where does that comes from?
Gv1, with comment 33 suggestion(s),
and some more.
Attachment #486089 - Attachment is obsolete: true
Attachment #486265 - Flags: review?(dean_tessman)
Attachment #486089 - Flags: review-
Comment on attachment 486265 [details] [diff] [review]
(Gv1a) /xpfe/components/directory/nsDirectoryViewer.cpp
[Checked in: See comment 46]

>+
>+  PRBool refireTimer = PR_FALSE;
>     // check both lists to see if the timer needs to continue firing
>     if (httpIndex->mConnectionList)

r=me after you fix this indentation.
Attachment #486265 - Flags: review?(dean_tessman) → review+
Comment on attachment 486265 [details] [diff] [review]
(Gv1a) /xpfe/components/directory/nsDirectoryViewer.cpp
[Checked in: See comment 46]

"approval2.0=?":
Simple code clean-up, no risk.
Attachment #486265 - Flags: approval2.0?
Comment on attachment 486595 [details] [diff] [review]
(Hv1) /extensions/*
[Checked in: Comment 47]

>diff --git a/extensions/cookie/nsCookiePromptService.cpp b/extensions/cookie/nsCookiePromptService.cpp
>--- a/extensions/cookie/nsCookiePromptService.cpp
>+++ b/extensions/cookie/nsCookiePromptService.cpp
>@@ -67,22 +67,20 @@ nsCookiePromptService::CookieDialog(nsID
>-  // since we're setting PRInt32's here, we have to sanitize the PRBool's first.
>-  // (myBool != PR_FALSE) is guaranteed to return either 1 or 0.
>   block->SetInt(nsICookieAcceptDialog::ACCEPT_COOKIE, 1);
>   block->SetString(nsICookieAcceptDialog::HOSTNAME, NS_ConvertUTF8toUTF16(aHostname).get());
>   block->SetInt(nsICookieAcceptDialog::COOKIESFROMHOST, aCookiesFromHost);
>-  block->SetInt(nsICookieAcceptDialog::CHANGINGCOOKIE, aChangingCookie != PR_FALSE);
>+  block->SetInt(nsICookieAcceptDialog::CHANGINGCOOKIE, aChangingCookie ? 1 : 0);
>   

I am not a reviewer here, but drive-by-comment [biesi may want to respond before you make any change though]. If the SetInt for COOKIESFROMHOST is ok-as-is, probably should use that style, since aCookiesFromHost is also a PRBool, if its not than you want to get it to also match.
(In reply to comment #41)
---errr---- ignore my last comment here, I was wrong :(
Attachment #486595 - Flags: review?(cbiesinger) → review+
Comment on attachment 486595 [details] [diff] [review]
(Hv1) /extensions/*
[Checked in: Comment 47]

"approval2.0=?":
Simple code clean-up, no risk.
Attachment #486595 - Flags: approval2.0?
Attachment #486265 - Flags: approval2.0? → approval2.0-
This is unwanted tree-churn now, please wait until after FF4 branches.
Attachment #486595 - Flags: approval2.0? → approval2.0-
Figuring out which parts of these patches should land on trunk and getting them on cedar is just too much work.  Please land on mozilla-central after coordinating with the sheriff.
Whiteboard: [good first bug] → [good first bug][not-ready-for-cedar]
Comment on attachment 486265 [details] [diff] [review]
(Gv1a) /xpfe/components/directory/nsDirectoryViewer.cpp
[Checked in: See comment 46]

http://hg.mozilla.org/mozilla-central/rev/d40935669248
Gv1a, with comment 38 suggestion(s).
Attachment #486265 - Attachment description: (Gv1a) /xpfe/components/directory/nsDirectoryViewer.cpp → (Gv1a) /xpfe/components/directory/nsDirectoryViewer.cpp [Checked in: See comment 46]
Comment on attachment 486595 [details] [diff] [review]
(Hv1) /extensions/*
[Checked in: Comment 47]

http://hg.mozilla.org/mozilla-central/rev/b79af914191d
Attachment #486595 - Attachment description: (Hv1) /extensions/* → (Hv1) /extensions/* [Checked in: Comment 47]

No activity for 8 years, I think we can close it

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Type: defect → task
You need to log in before you can comment on or make changes to this bug.