dblaikie added inline comments.
================
Comment at: include/clang/AST/Expr.h:5103
+ using reference = AssociationTy<Const>;
+ using pointer = AssociationTy<Const>;
+ AssociationIteratorTy() = default;
----------------
aaron.ballman wrote:
> riccibruno wrote:
> > aaron.ballman wrote:
> > > Carrying over the conversation from D57098:
> > >
> > > >> @aaron.ballman Cute, but I suspect this may come back to bite us at
> > > >> some point. For instance, if someone thinks they're working with a
> > > >> real pointer, they're likely to expect pointer arithmetic to work when
> > > >> it won't (at least they'll get compile errors though).
> > > > @riccibruno Hmm, but pointer is just the return type of operator-> no ?
> > > > Is it actually required to behave like a pointer ? The only requirement
> > > > I can find is that It->x must be equivalent to (*It).x, which is true
> > > > here.
> > >
> > > I double-checked and you're right, this is not a requirement of the STL.
> > >
> > > > Also looking at the requirements for forward iterators I think that
> > > > this iterator should actually be downgraded to an input iterator,
> > > > because of the requirement that reference = T&.
> > >
> > > My concern is that with the less functional iterators, common algorithms
> > > get more expensive. For instance, `std::distance()`, `std::advance()`
> > > become more expensive without random access, and things like
> > > `std::prev()` become impossible.
> > >
> > > It seems like the view this needs to provide is a read-only access to the
> > > `AssociationTy` objects (because we need to construct them on the fly),
> > > but not the data contained within them. If the only thing you can get
> > > back from the iterator is a const pointer/reference/value type, then we
> > > could store a local "current association" object in the iterator and
> > > return a pointer/reference to that. WDYT?
> > I am worried about lifetime issues with this approach. Returning a
> > reference/pointer to an `Association` object stored in the iterator means
> > that the reference/pointer will dangle as soon as the iterator goes out of
> > scope. This is potentially surprising since the "container" (that is the
> > `GenericSelectionExpr`) here will still be in scope. Returning a value is
> > safer in this aspect.
> >
> > I believe it should be possible to make the iterator a random access
> > iterator, at least
> > if we are willing to ignore some requirements:
> >
> > 1.) For forward iterators and up, we must have `reference = T&` or `const
> > T&`.
> > 2.) For forward iterators and up, `It1 == It2` if and only if `*It1` and
> > `*It2` are bound to the same object.
> > I am worried about lifetime issues with this approach. Returning a
> > reference/pointer to an Association object stored in the iterator means
> > that the reference/pointer will dangle as soon as the iterator goes out of
> > scope. This is potentially surprising since the "container" (that is the
> > GenericSelectionExpr) here will still be in scope. Returning a value is
> > safer in this aspect.
>
> That's valid.
>
> > I believe it should be possible to make the iterator a random access
> > iterator, at least if we are willing to ignore some requirements:
> >
> > 1.) For forward iterators and up, we must have reference = T& or const T&.
> > 2.) For forward iterators and up, It1 == It2 if and only if *It1 and *It2
> > are bound to the same object.
>
> Yes, but then passing these iterators to STL algorithms will have UB because
> we claim to meet the requirements for random access iteration but don't
> actually meet the requirements. I am not certain what problems might arise
> from violating these requirements.
>
> @dblaikie, @mclow.lists: do you have opinions on whether it's okay to not
> meet these requirements or suggestions on what we could do differently with
> the design?
My vote is usually "yeah, have a member inside the iterator you return a
reference/pointer to" to meet the iterator requirements. Yes, it means if you
keep a pointer/reference to it, that is invalidated when you increment the
iterator. But that's well established in iterator semantics.
(might be shooting from the hip here/not fully understanding the
nuances/tradeoffs in this case)
================
Comment at: include/clang/AST/Expr.h:5084
+ /// storage of Stmt * and TypeSourceInfo * in GenericSelectionExpr.
+ template <bool Const> class AssociationIteratorTy {
+ friend class GenericSelectionExpr;
----------------
Worth using any of the iterator helpers LLVM has? (iterator_facade or the like)
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57106/new/
https://reviews.llvm.org/D57106
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
