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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits