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

Fri Jan 25 05:47:34 PST 2019

riccibruno marked 2 inline comments as done.
riccibruno added inline comments.

================
Comment at: include/clang/AST/Expr.h:5103
+    using reference = AssociationTy<Const>;
+    using pointer = AssociationTy<Const>;
+    AssociationIteratorTy() = default;
----------------
dblaikie wrote:
> 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)
I believe there are 3 possibilities here:

Option 1 : Make the iterator an input iterator, and make `operator*` return a value.
Pros : Safe, compliant with the spec.
Cons : Morally we can do all of the operations of a random iterator.

Option 2 : Make the iterator a random access iterator, and make `operator*` return a value.
Pros : Safe wrt lifetime issues.
Cons : Do not complies with the two requirement of forward iterators mentioned above.

Option 3 : Make the iterator a random access iterator, and make `operator*` return a
reference to an object stored inside the iterator.
Pros : Compliant with the spec.
Cons : Nasty lifetime issues (see below)

I believe that option 3 is problematic. An example:

```
AssociationIterator It = ...;
const Association &Assoc = *It++;
// oops, Assoc is dangling since It++ returns a value,
// which goes out of scope at the end of the full expression.
// The same problem do not exists when operator* returns a
// value since the lifetime of the returned Association will be
// extended when bound to the reference.
```

Probably the safe thing to do is to go with option 1.

================
Comment at: include/clang/AST/Expr.h:5084
+  /// storage of Stmt * and TypeSourceInfo * in GenericSelectionExpr.
+  template <bool Const> class AssociationIteratorTy {
+    friend class GenericSelectionExpr;
----------------
dblaikie wrote:
> Worth using any of the iterator helpers LLVM has? (iterator_facade or the like)
I did try to use `iteratore_facade` but for some reason I was getting strange overload resolution failures with it.

In the end it did not save much and so I just rewrote the boiler-plate (especially given that if we end up going with an input iterator there is not going to be much boiler-plate).

Repository:
  rC Clang

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

https://reviews.llvm.org/D57106