398312 - PRBool misuse bugs in layout/

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[(dormant account)]

Attachment: First set of corrections for review

This is for the simple prbool usage errors in layout/

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The nsCSSRendering and nsCaret changes look bogus.  And the nsBlockReflowstate.h change could be simplified to skip the intermediate |result| variable.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In particular, ridgeGroove should be a PRUint8, and DrawAtpositionWithHint should just return PR_FALSE.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 283237 [details] [diff] [review]
First set of corrections for review

>   PRBool GetFlag(PRUint32 aFlag) const
>   {
>     NS_ASSERTION(aFlag<=BRS_LASTFLAG, "bad flag");
>-    PRBool result = (mFlags & aFlag);
>+    PRBool result = (0 != (mFlags & aFlag));
>     if (result) return PR_TRUE;
>     return PR_FALSE;

Of course, this should just return the (0 != ...) expression, not bloat source with a temporary and an if and two returns.... Preemptive sr=me with this hand-fix (or teach your stuff about it if you can ;-).

/be

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And nsDocumentViewer looks fine without the change -- I don't see why anything is needed.

Comment 5

[(dormant account)]

Attachment: Corrected issues

Modified patch according to comments.

(In reply to comment #3)
> Of course, this should just return the (0 != ...) expression, not bloat source
> with a temporary and an if and two returns.... Preemptive sr=me with this
> hand-fix (or teach your stuff about it if you can ;-).
> 
> /be
> 

I'm pretty sure you were joking. It would require more sophistication than reasonable.

Comment 6

[Jesse Ruderman]

Really?  It doesn't seem to me that it would be too hard to look for simple instances of the pattern

if (...)
  return true;
return false;

Comment 7

[Jesse Ruderman]

That's completely orthogonal from the type-correctness fixes that are the subject of this bug, though.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 283258 [details] [diff] [review]
Corrected issues

r=dbaron

Comment 9

[(dormant account)]

Attachment: Remainded of simple prbool violations in layout

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 283264 [details] [diff] [review]
Remainded of simple prbool violations in layout

>Index: layout/generic/nsFrame.cpp

>+  *aSelected = (PRBool)((0 != (mState & NS_FRAME_SELECTED_CONTENT)));

>+        rule->mDisplay = (PRBool)(0 != doDisplay);

Drop the (PRBool) in both of these, and the overparenthesization in the first.

>Index: layout/generic/nsInlineFrame.h

Keep the indentation of the ? and : so it's 2 chars after the start of the condition in both cases in this file (i.e., reindent, since you're moving the start of the condition).

r=dbaron with that

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #5)
> I'm pretty sure you were joking.

It was a parenthetical aside with a wink emoticon, after all.

> It would require more sophistication than reasonable.

Jesse's question stands, but we can take it up in email or a newsgroup.

/be

Comment 12

[(dormant account)]

Attachment: Combination of 2 previously reviews patches addressing simple prbool issues in layout/

Comment 13

[(dormant account)]

(In reply to comment #11)
> (In reply to comment #5)
> > I'm pretty sure you were joking.
> 
> It was a parenthetical aside with a wink emoticon, after all.
> 
> > It would require more sophistication than reasonable.
> 
> Jesse's question stands, but we can take it up in email or a newsgroup.

There are several "standard" compiler optimizations that would be cool to do at the source level. Jesse's example falls under Partial Redundancy Elimination. Another exciting one would be dead code elimination. It's "simple" to look for them, but  a little less obvious how to automatically rewrite them. Perhaps someone could even publish a paper on this. I haven't seen anything on adding common compiler optimizations to source2source tools.
We should continue this discussion on a relevant newsgroup (not sure which that is).

Taras

Comment 14

[(dormant account)]

Attachment: Combined prbool fix patch take 2. Using !! instead 0 !=

Updated this patch for consistency with !! in prbool conversions. David, is this ok with you?

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 283747 [details] [diff] [review]
Combined prbool fix patch take 2. Using !! instead 0 !=

I think I actually prefer 0 !=, but I think I'm outvoted, so this is fine.

Except:

>Index: layout/base/nsBidiPresUtils.cpp
>-        isBidiSystem = (hints & NS_RENDERING_HINT_ARABIC_SHAPING);
>+        isBidiSystem = (hints & NS_RENDERING_HINT_ARABIC_SHAPING);

You dropped the change here.

>Index: layout/generic/nsInlineFrame.h
>+    return !!((GetStateBits() & NS_INLINE_FRAME_BIDI_VISUAL_STATE_IS_SET)
>+                   ? (GetStateBits() & NS_INLINE_FRAME_BIDI_VISUAL_IS_LEFT_MOST)
>+                   : (!GetPrevInFlow()));

>+    return !!((GetStateBits() & NS_INLINE_FRAME_BIDI_VISUAL_STATE_IS_SET)
>+                   ? (GetStateBits() & NS_INLINE_FRAME_BIDI_VISUAL_IS_RIGHT_MOST)
>+                   : (!GetNextInFlow()));

It's probably clearer to put the !! around the then-part of the ?: rather than the whole thing.

r=dbaron with those fixed

Comment 16

[(dormant account)]

Attachment: Reviewed: prbool fix patch take 2. Using !! instead 0 !=

Changed patch as requested.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 284190 [details] [diff] [review]
Reviewed: prbool fix patch take 2. Using !! instead 0 !=

no need to add an extra set of parens around the changes in nsInlineFrame.h.

a1.9=dbaron with that

Comment 18

[(dormant account)]

Checking in layout/base/nsBidiPresUtils.cpp;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.cpp,v  <--  nsBidiPresUtils.cpp
new revision: 1.102; previous revision: 1.101
done
Checking in layout/base/nsCSSRendering.cpp;
/cvsroot/mozilla/layout/base/nsCSSRendering.cpp,v  <--  nsCSSRendering.cpp
new revision: 3.328; previous revision: 3.327
done
Checking in layout/base/nsCaret.cpp;
/cvsroot/mozilla/layout/base/nsCaret.cpp,v  <--  nsCaret.cpp
new revision: 1.181; previous revision: 1.180
done
Checking in layout/base/nsGenConList.cpp;
/cvsroot/mozilla/layout/base/nsGenConList.cpp,v  <--  nsGenConList.cpp
new revision: 3.4; previous revision: 3.3
done
Checking in layout/generic/nsBlockReflowState.h;
/cvsroot/mozilla/layout/generic/nsBlockReflowState.h,v  <--  nsBlockReflowState.h
new revision: 3.477; previous revision: 3.476
done
Checking in layout/generic/nsBulletFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBulletFrame.cpp,v  <--  nsBulletFrame.cpp
new revision: 1.166; previous revision: 1.165
done
Checking in layout/generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.760; previous revision: 3.759
done
Checking in layout/generic/nsIFrame.h;
/cvsroot/mozilla/layout/generic/nsIFrame.h,v  <--  nsIFrame.h
new revision: 3.379; previous revision: 3.378
done
Checking in layout/generic/nsInlineFrame.h;
/cvsroot/mozilla/layout/generic/nsInlineFrame.h,v  <--  nsInlineFrame.h
new revision: 1.69; previous revision: 1.68
done
Checking in layout/generic/nsLineLayout.h;
/cvsroot/mozilla/layout/generic/nsLineLayout.h,v  <--  nsLineLayout.h
new revision: 3.108; previous revision: 3.107
done
Checking in layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.296; previous revision: 3.295
done
Checking in layout/style/nsStyleContext.h;
/cvsroot/mozilla/layout/style/nsStyleContext.h,v  <--  nsStyleContext.h
new revision: 3.29; previous revision: 3.28
done
Checking in layout/style/nsStyleStruct.h;
/cvsroot/mozilla/layout/style/nsStyleStruct.h,v  <--  nsStyleStruct.h
new revision: 3.124; previous revision: 3.123
done
Checking in layout/svg/base/src/nsSVGGlyphFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGGlyphFrame.cpp,v  <--  nsSVGGlyphFrame.cpp
new revision: 1.111; previous revision: 1.110
done
Checking in layout/svg/base/src/nsSVGPatternFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGPatternFrame.cpp,v  <--  nsSVGPatternFrame.cpp
new revision: 1.57; previous revision: 1.56
done
Checking in layout/xul/base/src/nsBoxFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsBoxFrame.cpp,v  <--  nsBoxFrame.cpp
new revision: 1.339; previous revision: 1.338
done
Checking in layout/xul/base/src/nsSprocketLayout.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsSprocketLayout.cpp,v  <--  nsSprocketLayout.cpp
new revision: 1.61; previous revision: 1.60
done
Checking in layout/xul/base/src/tree/src/nsTreeContentView.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeContentView.cpp,v  <--  nsTreeContentView.cpp
new revision: 1.69; previous revision: 1.68
done