Closed Bug 398312 Opened 13 years ago Closed 13 years ago

PRBool misuse bugs in layout/

Categories

(Core :: Layout, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

References

Details

Attachments

(1 file, 5 obsolete files)

This is for the simple prbool usage errors in layout/
The nsCSSRendering and nsCaret changes look bogus.  And the nsBlockReflowstate.h change could be simplified to skip the intermediate |result| variable.
In particular, ridgeGroove should be a PRUint8, and DrawAtpositionWithHint should just return PR_FALSE.
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
Attachment #283237 - Flags: superreview+
And nsDocumentViewer looks fine without the change -- I don't see why anything is needed.
Attached patch Corrected issues (obsolete) — Splinter Review
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.
Assignee: nobody → tglek
Attachment #283237 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #283258 - Flags: review?
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;
That's completely orthogonal from the type-correctness fixes that are the subject of this bug, though.
Comment on attachment 283258 [details] [diff] [review]
Corrected issues

r=dbaron
Attachment #283258 - Flags: review? → review+
Attachment #283264 - Flags: review?(dbaron)
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
Attachment #283264 - Flags: review?(dbaron) → review+
(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


Attachment #283258 - Attachment is obsolete: true
Attachment #283264 - Attachment is obsolete: true
Attachment #283271 - Flags: approval1.9?
(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
Summary: first batch of prboolcheck bugs → PRBool misuse bugs in layout/
Updated this patch for consistency with !! in prbool conversions. David, is this ok with you?
Attachment #283271 - Attachment is obsolete: true
Attachment #283747 - Flags: review?(dbaron)
Attachment #283271 - Flags: approval1.9?
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
Attachment #283747 - Flags: review?(dbaron) → review+
Changed patch as requested.
Attachment #283747 - Attachment is obsolete: true
Attachment #284190 - Flags: approval1.9?
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
Attachment #284190 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.