[PATCH] D27874: ilist_iterator: Allow conversion between reverse and forward iterators

[Matthias Braun via llvm-commits]

> On Jan 30, 2017, at 10:09 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Mon, Jan 30, 2017 at 10:00 AM Duncan P. N. Exon Smith <dexonsmith at apple.com <mailto:dexonsmith at apple.com>> wrote:
> I thing we should have *something* that does the natural thing,
> 
> By 'natural thing' you mean the standard-esque off-by-one thing?
At least to me the intuitive/natural behavior was to stay on the same object (current getReverse()). Probably similar to other people who haven't been using conversion from/to reverse std::reverse_iterator before. Though thinking about it/discussing this more, the STL like behavior does make sense: You want a conversion that works on any valid iterator and that includes end()/rend().

IMO:
- If we add constructors we should implement the off-by-one behavior.
- I would have the tendency to not add any constructors to avoid problems with mismatched expectations, then again if someone has a good use for the functionality we should add it.
- I would not change the getReverse() behavior (why would you have a 2nd function that does exactly the same as the constructor?).
- We could however be more aggressive and remove getReverse(). At least in my code I ended up with the following pattern which works the same (for ilist_nodes), so maybe getReverse() is unnecessary:

    ilist_iterator I = ...;
    auto R = I->getReverseIterator(); // dereference forward iterator and get reverse iterator to same object

- Matthias

>  
> and I'm extremely hesitant to change the semantics of getReverse() since that was extremely error prone last time.  Here's another possibility:
> - Rename getReverse() to some-great-new-name-that-is-clear.
> - Add conversions between forward/reverse do the same thing as std::reverse_iterator, with the off-by-1 thing.
> 
> Any suggestions for something that's clear?
> 
> > On 2017-Jan-30, at 09:49, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
> >
> > Maybe - I'm not sure the explicit ctor is sufficiently explicit from a self-documenting code perspective:
> >
> > reverse_iterator R(I);
> > reverse_iterator R = I.getReverse();
> >
> > I would be a bit surprised that those two things had different semantics.
> >
> > On Mon, Jan 30, 2017 at 9:47 AM Duncan P. N. Exon Smith <dexonsmith at apple.com <mailto:dexonsmith at apple.com>> wrote:
> > I think adding:
> > --
> > explicit iterator(reverse_iterator I) : I(++I.getReverse().base()) {}
> > explicit reverse_iterator(iterator I) : I(++I.getReverse().base()) {}
> > --
> > should fix the problem.
> > - getReverse() keeps the current semantics of getting the reverse of whatever you're currently referencing.
> > - conversions between forward/reverse do the same thing as std::reverse_iterator, with the off-by-1 thing.
> >
> >
> >
> > > On 2017-Jan-30, at 09:43, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
> > >
> > > Yeah, I think it might be too subtle to have these reverse iterators behave differently from other reverse iterators. (I haven't checked the spec in detail to see if it's only stD::reverse_iterator that has this oddity, or if all reverse iterators must do so) 'getReverse' seems like a good/clear solution, if a little odd but at least visibly so.
> > >
> > > On Wed, Jan 25, 2017 at 3:33 PM Matthias Braun via Phabricator via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
> > > MatzeB abandoned this revision.
> > > MatzeB added a comment.
> > >
> > > I wasn't aware of the subtle shift-by-1 semantics when converting to std::reverse_iterator (must have missed the getReverse() comment). If we go with that semantics I would need extra std::next/std::prev calls in my code anyway which doesn't make it much simpler compared to MyIterator->getReverseIterator(). Going for semantics different from STL is not a good idea IMO.
> > >
> > >
> > > Repository:
> > >   rL LLVM
> > >
> > > https://reviews.llvm.org/D27874 <https://reviews.llvm.org/D27874>
> > >
> > >
> > >
> > > _______________________________________________
> > > llvm-commits mailing list
> > > llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
> >
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170130/0942a02e/attachment.html>