Clang build warning (using clang 3.8): { layout/generic/nsRubyBaseContainerFrame.cpp:774:5: warning: loop will run at most once (loop increment never executed) [-Wunreachable-code-loop-increment] for (nsIFrame* frame : aColumn) { ^~~ } Larger snippet of code: > nsBlockFrame* oldFloatCB = nullptr; > for (nsIFrame* frame : aColumn) { > oldFloatCB = nsLayoutUtils::GetFloatContainingBlock(frame); > break; > } This was added here: https://hg.mozilla.org/mozilla-central/rev/96478aae9d0e#l1.58 xidorn, do you know if this code is actually doing what you intended it to do? (If so, perhaps it should be rewritten using an "if" check, to avoid the clang warning & the false impression that we're looping?)

It is doing what I intended to do. A ruby column may lack frame in any level, and the iterator of RubyColumn goes through all existing frames (skips ruby levels with no frame). This loop is for invoking that iterating logic and pick the first existing frame. Probably it can be rewritten to something like > nsBlockFrame* oldFloatCB = nsLayoutUtils::GetFloatContainingBlock(*aColumn.begin()); Actually shorter than current... okay. I used for-loop because it feels native to use that with iterator, but yeah, it isn't necessary.

Comment on attachment 8815997 [details] Bug 1321394 - Remove unnecessary loop in nsRubyBaseContainerFrame. https://reviewboard.mozilla.org/r/96756/#review97292 ::: layout/generic/nsRubyBaseContainerFrame.cpp:772 (Diff revision 1) > } else { > // We are not pulling an intra-level whitespace, which means all > // elements we are going to pull can have non-whitespace content, > // which may contain float which we need to reparent. > - nsBlockFrame* oldFloatCB = nullptr; > - for (nsIFrame* frame : aColumn) { > + nsBlockFrame* oldFloatCB = > + nsLayoutUtils::GetFloatContainingBlock(*aColumn.begin()); It's not clear to me whether, at this point in the code, it's guaranteed that aColumn.begin() will return... - a dereffable iterator (i.e. one that is not equal to aColumn.end()) - which will produce a non-null frame when dereferenced Are these things guaranteed? (As you said, the iterator returned by begin() skips levels with no frame. Is it possible that *no* levels have a frame here?) If we have the guarantees that I'm asking about, then I agree this patch is a valid simplification (and the patch would benefit from a comment and/or assertion to make this guarantee/expectation a little more concrete). If we don't have the guarantee, though, then it seems like nsLayoutUtils::GetFloatContainingBlock will crash on a null deref. (It dereferences the frame pointer that's passed in, as its first step.)

Comment on attachment 8815997 [details] Bug 1321394 - Remove unnecessary loop in nsRubyBaseContainerFrame. https://reviewboard.mozilla.org/r/96756/#review97292 > It's not clear to me whether, at this point in the code, it's guaranteed that aColumn.begin() will return... > - a dereffable iterator (i.e. one that is not equal to aColumn.end()) > - which will produce a non-null frame when dereferenced > > Are these things guaranteed? (As you said, the iterator returned by begin() skips levels with no frame. Is it possible that *no* levels have a frame here?) > > If we have the guarantees that I'm asking about, then I agree this patch is a valid simplification (and the patch would benefit from a comment and/or assertion to make this guarantee/expectation a little more concrete). > > If we don't have the guarantee, though, then it seems like nsLayoutUtils::GetFloatContainingBlock will crash on a null deref. (It dereferences the frame pointer that's passed in, as its first step.) There shouldn't be a column with no frame at all, and the "no frame" actually means nullptr in the column array, so it never returns a null frame. These two guarantees are also needed for the code before this simplification: 1. The assertion below ensures that the loop body runs at least once, so oldFloatCB can be something other than nullptr. 2. If the iterator can return nullptr, GetFloatContainingBlock is already doing null-dereference. So this simplification doesn't change any of the needed guarantees.

Comment on attachment 8815997 [details] Bug 1321394 - Remove unnecessary loop in nsRubyBaseContainerFrame. https://reviewboard.mozilla.org/r/96756/#review97292 > There shouldn't be a column with no frame at all, and the "no frame" actually means nullptr in the column array, so it never returns a null frame. These two guarantees are also needed for the code before this simplification: > 1. The assertion below ensures that the loop body runs at least once, so oldFloatCB can be something other than nullptr. > 2. If the iterator can return nullptr, GetFloatContainingBlock is already doing null-dereference. > > So this simplification doesn't change any of the needed guarantees. Also there is an assertion in RubyColumn::Iterator::operator*() that it doesn't return an nullptr. So everything is still enforced by assertion after this change. No null-dereference should happen without an assertion.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4) > 1. The assertion below ensures that the loop body runs at least once, so > oldFloatCB can be something other than nullptr. True, ok. (This is the main part I wasn't sure about -- do we know the loop is always entered? I guess we do, given this assertion.)

Comment on attachment 8815997 [details] Bug 1321394 - Remove unnecessary loop in nsRubyBaseContainerFrame. https://reviewboard.mozilla.org/r/96756/#review97300 r=me with an assertion added, per nit below. ::: layout/generic/nsRubyBaseContainerFrame.cpp:771 (Diff revision 1) > - nsBlockFrame* oldFloatCB = nullptr; > - for (nsIFrame* frame : aColumn) { > + nsBlockFrame* oldFloatCB = > + nsLayoutUtils::GetFloatContainingBlock(*aColumn.begin()); I'll take your word for it that column is non-empty here -- but we should make that clear to future readers of this code, too. Right now it looks like a potential invalid-memory-read [if it were possible for aColumn to be empty]. Could you add... MOZ_ASSERT(aColumn.begin() != aColumn.end(), ...) ...just before this line? If that assertion were to fail, we'd be doing an out-of-bounds read (and potentially crash unsafely). So it's something worth worrying about, and it's worth making it clear that it's a scenario we've considered & rule out as being impossible (with verification). (As you noted, the MOZ_ASSERT(oldFloatCB, ...) kind of checks for this, in current mozilla-central. But after your changes, if we were to end up with an empty aColumn (whose begin()==end()), then we'd now crash before we ever get to that MOZ_ASSERT(oldFloatCB) line. So that existing assertion is "too late" for warning us about this condition.)

Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/308bc1917b91 Remove unnecessary loop in nsRubyBaseContainerFrame. r=dholbert