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

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Comment 1

[Daniel Holbert [:dholbert]]

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

Comment 2

[Bernd]

display->mBreakAfter || (prevRg && prevRg->HasInternalBreakAfter())
should be 
(display->mBreakAfter || (prevRg && prevRg->HasInternalBreakAfter()))

Comment 3

[Daniel Holbert [:dholbert]]

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)

Comment 4

[Bernd]

Attachment: patch

Comment 5

[Daniel Holbert [:dholbert]]

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?

Comment 6

[Daniel Holbert [:dholbert]]

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

Comment 7

[Bernd]

Attachment: hg export for pushing

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.

Comment 8

[Bernd]

this did pass the tryserver

Comment 9

[Daniel Holbert [:dholbert]]

Pushed bernd's "hg export" attachment (with the bug number in the checkin comment updated to be this bug, rather than bug 558574)
http://hg.mozilla.org/mozilla-central/rev/1fb9ef691b5c