[PATCH] D57106: [AST] Introduce GenericSelectionExpr::Association

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: dblaikie, mclow.lists.
aaron.ballman added subscribers: dblaikie, mclow.lists.
aaron.ballman added a comment.

Adding some additional reviewers to help tease out the iterator design questions.



================
Comment at: include/clang/AST/Expr.h:5103
+    using reference = AssociationTy<Const>;
+    using pointer = AssociationTy<Const>;
+    AssociationIteratorTy() = default;
----------------
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?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57106/new/

https://reviews.llvm.org/D57106