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