Closed Bug 567835 Opened 12 years ago Closed 12 years ago

"warning: suggest parentheses around && within ||" in nsTableFrame.cpp, after bug 558574


(Core :: Layout: Tables, defect)

Not set





(Reporter: dholbert, Assigned: bernd_mozilla)




(2 files)

No description provided.
[argh, I hit 'enter' while typing bug summary, & accidentally filed bug without a description]

Bug 558574 introduced these build warnings, in a previously warning-free directory:
./../../mozilla/layout/tables/nsTableFrame.cpp: In static member function ‘static PRBool nsTableFrame::PageBreakAfter(nsIFrame*, nsIFrame*)’:
../../../mozilla/layout/tables/nsTableFrame.cpp:296: warning: suggest parentheses around && within ||
../../../mozilla/layout/tables/nsTableFrame.cpp:306: warning: suggest parentheses around && within ||

The flagged blocks of code are:
>294   // don't allow a page break after a repeated element ...
>295   if (display->mBreakAfter || (prevRg && prevRg->HasInternalBreakAfter()) &&
>296       !IsRepeatedFrame(aSourceFrame)) {
>297     return !(aNextFrame && IsRepeatedFrame(aNextFrame)); // or before
>302     // don't allow a page break before a repeated element ...
>303      nsTableRowGroupFrame* nextRg = do_QueryFrame(aNextFrame);
>304     if (display->mBreakBefore ||
>305         (nextRg && nextRg->HasInternalBreakBefore()) &&
>306         !IsRepeatedFrame(aNextFrame)) {
>307       return !IsRepeatedFrame(aSourceFrame); // or after

In both cases, the "&& !IsRepeatedFrame" is the operator that triggers the warning.

This operator is lower-precedence than the initial "||" -- i.e. we're behaving as if there were an extra set of parenthesis *after* the "||".  (and the warning suggests adding those parens to be more explicit)

Is the current behavior what we actually want?  (Or should there be parens around the ||, before the final &&?)  I haven't looked at the code too thoroughly, so I'm not sure.
Blocks: 558574
OS: Linux → All
Summary: "suggest parentheses around && within ||" → "warning: suggest parentheses around && within ||" in nsTableFrame.cpp, after bug 558574
display->mBreakAfter || (prevRg && prevRg->HasInternalBreakAfter())
should be 
(display->mBreakAfter || (prevRg && prevRg->HasInternalBreakAfter()))
Assignee: nobody → bernd_mozilla
That change (adding parens around the operands of "||") makes sense to me -- that was what I suspected was intended.

(We need the same change for the "display->mBreakBefore" chunk, too, of course)
Hardware: x86 → All
Attached patch patchSplinter Review
Attachment #447466 - Flags: review?(dholbert)
Attachment #447466 - Flags: review?(dholbert) → review+
Bernd: I'm going to be pushing a few "checkin-needed" patches later today, and I'd be happy to include this bug's patch in that push (with you as the author of course) -- is that ok by you, or would you rather land it yourself?
(nevermind @ comment 5 -- I pushed the other patches without this one.  I'd still be happy to check in this patch & watch the tree if you'd like, though.)
changeset for pushing, I just submitted it to try server.
I did not want to ask for checkin without pushing it over tryserver first.

But in general: checkin  in to the permaorange takes a solid  second place in Mozilla top annoyances (first is waiting endless for review). So whoever checks in my stuff I am thankful for this.

The fun of digging, debugging and interaction with brilliant people outweighs for me those annoyances.
this did pass the tryserver
Pushed bernd's "hg export" attachment (with the bug number in the checkin comment updated to be this bug, rather than bug 558574)
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.