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

[Kyle Butt via llvm-commits]

>
> 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.


This isn't true. std::set, std::map, std::list also do the off by one
accounting.


> 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...
>
>
When dealing with a range, off by one makes perfect sense. When you need to
shrink from the front, you work from [begin, end)
Then to start working from the back you flip the iterators and now you have
(rend, rbegin]


> > On 2016-Dec-20, at 13:43, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > 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/b9c1125f/attachment.html>