riccibruno 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:
> > 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.
> > Probably the safe thing to do is to go with option 1.
>
> In the interests of solving the problem at hand, I think we should go with
> the safest option and can put a FIXME near the `iterator_category` that
> explains why it's an input iterator and that we'd like it to have stronger
> guarantees someday. It should meet our needs for the AST dumping refactoring;
> we can solve the harder problems if/when they arise in practice.
Let's do this. I tried to summarize how we ended up with an input iterator.
================
Comment at: lib/AST/ASTDumper.cpp:1465
- for (unsigned I = 0, N = E->getNumAssocs(); I != N; ++I) {
+ for (auto Assoc : E->associations()) {
dumpChild([=] {
----------------
aaron.ballman wrote:
> I believe `const auto &` should be safe here even with the call to
> `dumpChild()` because the lambda captures `Assoc` by copy (even if the
> iterator does not return a reference, the value will be lifetime extended
> because of the const reference). Same applies elsewhere.
Yes it is safe. Done.
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
