228448 - Remove '=='/'!=' 'PR_FALSE'/'PR_TRUE'

Status: RESOLVED FIXED

(Core :: General, task)

Description

[Serge Gautherie (:sgautherie)]

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>

Comment 1

[Serge Gautherie (:sgautherie)]

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>

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <nsInternetSearchService.cpp>

Also:
*Removes unneeded brackets of '{ // 1 line }' blocks.
*Separates 'if( (rv = xyz) )' into 'rv = xyz; if(rv)'.
*Does 2 ' ? : ' code rewrites.

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 137404 [details] [diff] [review]
(Av1) <nsInternetSearchService.cpp>


'r=?': (see comment 2)
Can you (super-)review, compile, test, check it in ?

Comment 4

[Alec Flett]

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.

Comment 5

[[e-mail]]

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).

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) <nsInternetSearchService.cpp> [Checked in: Comment 11]

Av1, with comment 5 suggestions.

Comment 7

[Serge Gautherie (:sgautherie)]

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.

Comment 8

[Alec Flett]

I will not review patches until they have been compiled and tested. Let me know
when that happens.

Comment 9

[[e-mail]]

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 10

[Alec Flett]

Comment on attachment 137796 [details] [diff] [review]
(Av2) <nsInternetSearchService.cpp>
[Checked in: Comment 11]

sr=alecf then.

Comment 11

[Serge Gautherie (:sgautherie)]

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 }

Comment 12

[Serge Gautherie (:sgautherie)]

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 13

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) </mailnews/addrbook/src/ns*.*>

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 138132 [details] [diff] [review]
(Bv1) </mailnews/addrbook/src/ns*.*>


I have no compiler: Could you compile/test/review it ? Thanks.

Comment 15

[[e-mail]]

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 (

Comment 16

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1b) </mailnews/addrbook/src/ns*.*> [Checked in: Comment 18]

Bv1, with comment 15 suggestion(s).

Comment 17

[Serge Gautherie (:sgautherie)]

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+  }

Comment 18

[Serge Gautherie (:sgautherie)]

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 }

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) <nsBookmarksService.cpp>

[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.

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) <nsNSSComponent.cpp>

[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.

Comment 21

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1a) </security/manager/ssl/src/ns*.cpp> [Checkin: Comment 23]

[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.

Comment 22

[Kai Engert [:KaiE:]]

Comment on attachment 333133 [details] [diff] [review]
(Dv1a) </security/manager/ssl/src/ns*.cpp>
[Checkin: Comment 23]

r=kaie, thanks!

Comment 23

[Reed Loden [:reed]]

Dv1a pushed in 17116:0380e2e6598a.

Comment 24

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1a-SM) <nsBookmarksService.cpp> [Obsoleted by bug 580663]

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...

Comment 25

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1-CC) </mailnews/*> except 1 [Checkin: Comment 27]

Comment 26

[David :Bienvenu]

Comment on attachment 371843 [details] [diff] [review]
(Ev1-CC) </mailnews/*> except 1
[Checkin: Comment 27]

thx for the patch.

Comment 27

[Serge Gautherie (:sgautherie)]

Comment on attachment 371843 [details] [diff] [review]
(Ev1-CC) </mailnews/*> except 1
[Checkin: Comment 27]


http://hg.mozilla.org/comm-central/rev/9c76df358a51

Comment 28

[Tyler Downer [He/Him]]

Sooo, this should be marked FIXED?

Comment 29

[Serge Gautherie (:sgautherie)]

(In reply to comment #28)
> Sooo, this should be marked FIXED?

Nooo, this remains to BE fixed!

Comment 30

[Serge Gautherie (:sgautherie)]

Current status is "Found 307 matching lines in 112 files".

Comment 31

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1-CC) /mailnews/compose/src/nsMsgCompose.cpp [Checked in: Comment 34]

Comment 32

[Serge Gautherie (:sgautherie)]

Attachment: (Gv1) /xpfe/components/directory/nsDirectoryViewer.cpp

Comment 33

[:deanis74]

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

Comment 34

[Serge Gautherie (:sgautherie)]

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

Comment 35

[Justin Wood (:Callek)]

(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.

Comment 36

[Serge Gautherie (:sgautherie)]

(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?

Comment 37

[Serge Gautherie (:sgautherie)]

Attachment: (Gv1a) /xpfe/components/directory/nsDirectoryViewer.cpp [Checked in: See comment 46]

Gv1, with comment 33 suggestion(s),
and some more.

Comment 38

[:deanis74]

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.

Comment 39

[Serge Gautherie (:sgautherie)]

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.

Comment 40

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1) /extensions/* [Checked in: Comment 47]

Comment 41

[Justin Wood (:Callek)]

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.

Comment 42

[Justin Wood (:Callek)]

(In reply to comment #41)
---errr---- ignore my last comment here, I was wrong :(

Comment 43

[Serge Gautherie (:sgautherie)]

Comment on attachment 486595 [details] [diff] [review]
(Hv1) /extensions/*
[Checked in: Comment 47]

"approval2.0=?":
Simple code clean-up, no risk.

Comment 44

[Benjamin Smedberg]

This is unwanted tree-churn now, please wait until after FF4 branches.

Comment 45

[(no longer active)]

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.

Comment 46

[Serge Gautherie (:sgautherie)]

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).

Comment 47

[Serge Gautherie (:sgautherie)]

Comment on attachment 486595 [details] [diff] [review]
(Hv1) /extensions/*
[Checked in: Comment 47]

http://hg.mozilla.org/mozilla-central/rev/b79af914191d

Comment 48

[Sylvestre Ledru [:Sylvestre]]

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