<div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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><br></div><div>This isn't true. std::set, std::map, std::list also do the off by one accounting.<br><div class="gmail_extra"><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
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>
<div class="gmail-HOEnZb"><div class="gmail-h5"><br></div></div></blockquote><div><br></div><div>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>Then to start working from the back you flip the iterators and now you have (rend, rbegin]</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><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>
><br>
> 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>
><br>
> 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>
><br>
> 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>
> iteratee added inline comments.<br>
><br>
><br>
> ================<br>
> Comment at: lib/CodeGen/IfConversion.cpp:<wbr>646-647<br>
> // Now, in preparation for counting duplicate instructions at the ends of the<br>
> - // blocks, move the end iterators up past any branch instructions.<br>
> - --TIE;<br>
> - --FIE;<br>
> -<br>
> - // After this point TIB and TIE define an inclusive range, which means that<br>
> - // TIB == TIE is true when there is one more instruction to consider, not at<br>
> - // the end. Because we may not be able to go before TIB, we need a flag to<br>
> - // indicate a completely empty range.<br>
> - bool TEmpty = false, FEmpty = false;<br>
> -<br>
> - // Upon exit TIE and FIE will both point at the last non-shared instruction.<br>
> - // They need to be moved forward to point past the last non-shared<br>
> - // instruction if the range they delimit is non-empty.<br>
> - auto IncrementEndIteratorsOnExit = make_scope_exit([&]() {<br>
> - if (!TEmpty)<br>
> - ++TIE;<br>
> - if (!FEmpty)<br>
> - ++FIE;<br>
> - });<br>
> + // blocks, switch to reverse_iterators. Note that these are NOT std<br>
> + // reverse_iterators, so we have to manually do the off by one accounting.<br>
> + MachineBasicBlock::reverse_<wbr>iterator RTIE = ++(TIE.getReverse());<br>
> ----------------<br>
> dblaikie wrote:<br>
> > 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>
> ><br>
> > 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>
> See the comment in include/llvm/ADT/ilist_<wbr>iterator.h:<br>
><br>
> /// Convert from an iterator to its reverse.<br>
> ///<br>
> /// TODO: Roll this into the implicit constructor once we're sure that no one<br>
> /// is relying on the std::reverse_iterator off-by-one semantics.<br>
><br>
> I take that to mean that we are intentionally not supplying the std::reverse_iterator semantics.<br>
><br>
> The comment is aimed at a reader familiar with those semantics.<br>
><br>
><br>
> <a href="https://reviews.llvm.org/D27792" rel="noreferrer" target="_blank" class="gmail-cremed cremed">https://reviews.llvm.org/<wbr>D27792</a><br>
><br>
><br>
><br>
<br>
</div></div></blockquote></div><br></div></div></div>