[PATCH] D27792: IfConversion: Use reverse_iterator to simplify. NFC

[David Blaikie via llvm-commits]

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?

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)

On Tue, Dec 20, 2016 at 1:01 PM Kyle Butt via Phabricator <
reviews at reviews.llvm.org> wrote:

> iteratee added inline comments.
>
>
> ================
> Comment at: lib/CodeGen/IfConversion.cpp:646-647
>    // Now, in preparation for counting duplicate instructions at the ends
> of the
> -  // blocks, move the end iterators up past any branch instructions.
> -  --TIE;
> -  --FIE;
> -
> -  // After this point TIB and TIE define an inclusive range, which means
> that
> -  // TIB == TIE is true when there is one more instruction to consider,
> not at
> -  // the end. Because we may not be able to go before TIB, we need a flag
> to
> -  // indicate a completely empty range.
> -  bool TEmpty = false, FEmpty = false;
> -
> -  // Upon exit TIE and FIE will both point at the last non-shared
> instruction.
> -  // They need to be moved forward to point past the last non-shared
> -  // instruction if the range they delimit is non-empty.
> -  auto IncrementEndIteratorsOnExit = make_scope_exit([&]() {
> -    if (!TEmpty)
> -      ++TIE;
> -    if (!FEmpty)
> -      ++FIE;
> -  });
> +  // blocks, switch to reverse_iterators. Note that these are NOT std
> +  // reverse_iterators, so we have to manually do the off by one
> accounting.
> +  MachineBasicBlock::reverse_iterator RTIE = ++(TIE.getReverse());
> ----------------
> dblaikie wrote:
> > 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?)
> >
> > 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?
> See the comment in include/llvm/ADT/ilist_iterator.h:
>
>   /// Convert from an iterator to its reverse.
>   ///
>   /// TODO: Roll this into the implicit constructor once we're sure that
> no one
>   /// is relying on the std::reverse_iterator off-by-one semantics.
>
> I take that to mean that we are intentionally not supplying the
> std::reverse_iterator semantics.
>
> The comment is aimed at a reader familiar with those semantics.
>
>
> https://reviews.llvm.org/D27792
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161220/0811e25d/attachment.html>