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

[Kyle Butt via llvm-commits]

On Tue, Dec 20, 2016 at 5:55 PM, Duncan P. N. Exon Smith via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

>
> On 2016-Dec-20, at 15:44, Kyle Butt <iteratee at google.com> wrote:
>
> 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.
>
>
> 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.
>
> I stand by my point that the new invalidation semantics are strictly
> better.  But being inconsistent is awkward...
>
> 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]
>
>
> It's the invalidation that's hard to reason about.
>
>
I think you're referring to this invalidation behavior?

http://stackoverflow.com/questions/19826255/why-does-insert-invalidate-the-stdset-reverse-iterator

If you have to make reverse_iterator a wrapper, you have to do this.

There's a middle ground where end().reverse() will give you rbegin(), but
has the invalidation semantics that you want.
I think that is actually the optimal situation.

In either case, I think that this discussion shouldn't impede the patch.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170105/f9039003/attachment.html>