<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 2016-Dec-20, at 15:44, Kyle Butt <<a href="mailto:iteratee@google.com" class="">iteratee@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">Normally, reverse iterators are not off-by-one. See the reverse iterators provided by containers like std::set, std::map, std::list, etc.. All of these have a handle to the same node that is getting dereferenced. They work by having a sentinel, rend(), that's past the final (reverse) element. They have similar guarantees to the forward iterators: they are only invalidated when the pointed-to-node gets deleted.</blockquote><div class=""><br class=""></div><div class="">This isn't true. std::set, std::map, std::list also do the off by one accounting.<br class=""></div></div></div></blockquote><div><br class=""></div><div>Huh... I stand corrected. Not sure how I got that :/. Regretfully, I had this incorrect impression when I was working on this, and probably convinced others of it.</div><div><br class=""></div><div>I stand by my point that the new invalidation semantics are strictly better. But being inconsistent is awkward...</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class="gmail_extra"><div class="gmail_quote"><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">
I haven't looked at this patch; from the limited context here, I don't understand why you'd *want* off-by-one accounting. Off-by-one accounting is usually complicated and hard to reason about...<br class="">
<div class="gmail-HOEnZb"><div class="gmail-h5"><br class=""></div></div></blockquote><div class=""><br class=""></div><div class="">When dealing with a range, off by one makes perfect sense. When you need to shrink from the front, you work from [begin, end)</div><div class="">Then to start working from the back you flip the iterators and now you have (rend, rbegin]</div></div></div></div></div></div></blockquote><div><br class=""></div><div>It's the invalidation that's hard to reason about.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote"><div class="gmail-HOEnZb"><div class="gmail-h5">
> On 2016-Dec-20, at 13:43, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="gmail-cremed cremed">dblaikie@gmail.com</a>> wrote:<br class="">
><br class="">
> Duncan - looks like you were working on ilist when you added this comment about the reverse iterator - any particular insight that might be relevant here?<br class="">
><br class="">
> Conversely, Kyle - is the comment needed? If getReverse gives a reverse iterator that points to the current element of the original iterator, it seems like that'd be obvious & not require a comment (& perhaps that was the point of the ilist API change there)? (If I'm understanding correctly)<br class="">
><br class="">
> On Tue, Dec 20, 2016 at 1:01 PM Kyle Butt via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="gmail-cremed cremed">reviews@reviews.llvm.org</a>> wrote:<br class="">
> iteratee added inline comments.<br class="">
><br class="">
><br class="">
> ================<br class="">
> Comment at: lib/CodeGen/IfConversion.cpp:<wbr class="">646-647<br class="">
> // Now, in preparation for counting duplicate instructions at the ends of the<br class="">
> - // blocks, move the end iterators up past any branch instructions.<br class="">
> - --TIE;<br class="">
> - --FIE;<br class="">
> -<br class="">
> - // After this point TIB and TIE define an inclusive range, which means that<br class="">
> - // TIB == TIE is true when there is one more instruction to consider, not at<br class="">
> - // the end. Because we may not be able to go before TIB, we need a flag to<br class="">
> - // indicate a completely empty range.<br class="">
> - bool TEmpty = false, FEmpty = false;<br class="">
> -<br class="">
> - // Upon exit TIE and FIE will both point at the last non-shared instruction.<br class="">
> - // They need to be moved forward to point past the last non-shared<br class="">
> - // instruction if the range they delimit is non-empty.<br class="">
> - auto IncrementEndIteratorsOnExit = make_scope_exit([&]() {<br class="">
> - if (!TEmpty)<br class="">
> - ++TIE;<br class="">
> - if (!FEmpty)<br class="">
> - ++FIE;<br class="">
> - });<br class="">
> + // blocks, switch to reverse_iterators. Note that these are NOT std<br class="">
> + // reverse_iterators, so we have to manually do the off by one accounting.<br class="">
> + MachineBasicBlock::reverse_<wbr class="">iterator RTIE = ++(TIE.getReverse());<br class="">
> ----------------<br class="">
> dblaikie wrote:<br class="">
> > Is this something we should be relying on? Does the spec say how reverse iterators can/should be constructed from non-reverse iterators in general? (and is MBB violating that contract?)<br class="">
> ><br class="">
> > Should we be fixing MBB's reverse_iterators to do this accounting if it's the usual/expected/precedential (which is totally a word) thing?<br class="">
> See the comment in include/llvm/ADT/ilist_<wbr class="">iterator.h:<br class="">
><br class="">
> /// Convert from an iterator to its reverse.<br class="">
> ///<br class="">
> /// TODO: Roll this into the implicit constructor once we're sure that no one<br class="">
> /// is relying on the std::reverse_iterator off-by-one semantics.<br class="">
><br class="">
> I take that to mean that we are intentionally not supplying the std::reverse_iterator semantics.<br class="">
><br class="">
> The comment is aimed at a reader familiar with those semantics.<br class="">
><br class="">
><br class="">
> <a href="https://reviews.llvm.org/D27792" rel="noreferrer" target="_blank" class="gmail-cremed cremed">https://reviews.llvm.org/<wbr class="">D27792</a><br class="">
><br class="">
><br class="">
><br class="">
<br class="">
</div></div></blockquote></div><br class=""></div></div></div>
</div></blockquote></div><br class=""></body></html>