riccibruno added inline comments.
================
Comment at: include/clang/AST/Expr.h:5068
+ Association getAssociation(unsigned I) const {
+ return Association(cast<Expr>(SubExprs[END_EXPR + I]), AssocTypes[I],
----------------
riccibruno wrote:
> aaron.ballman wrote:
> > steveire wrote:
> > > aaron.ballman wrote:
> > > > steveire wrote:
> > > > > aaron.ballman wrote:
> > > > > > Rather than gin these objects up on demand every time they're
> > > > > > needed, I'd prefer to see the class store `Association` objects
> > > > > > directly. I don't think it will be easy to do that and still
> > > > > > support `getAssocExprs()` and `getAssocTypeSourceInfos()`, so I
> > > > > > think those APIs should be removed in favor of this one. There's
> > > > > > currently not many uses of `getAssocExprs()` or
> > > > > > `getAssocTypeSourceInfos()` (I spot one each in Clang) so migration
> > > > > > to the new API should not be onerous.
> > > > > >
> > > > > > This should also have a range-based accessor version so that users
> > > > > > aren't required to use iterative loops to access the information (a
> > > > > > lot of the places you're already touching could use that
> > > > > > range-based interface).
> > > > > I would prefer that too, but it doesn't seem to be possible. This is
> > > > > a sub-range of the `SubExprs` returned from `children()`.
> > > > >
> > > > > In theory, that could be split into two members, but then you would
> > > > > need a range library to recombine them and implement `children()`:
> > > > > https://godbolt.org/z/ZVamdC
> > > > >
> > > > > This seems to be the best approach for now, and AFAIK it excludes a
> > > > > range-accessor.
> > > > > I would prefer that too, but it doesn't seem to be possible. This is
> > > > > a sub-range of the SubExprs returned from children().
> > > >
> > > > Ugh, you're right. :-(
> > > >
> > > > > In theory, that could be split into two members, but then you would
> > > > > need a range library to recombine them and implement children():
> > > > > https://godbolt.org/z/ZVamdC
> > > >
> > > > We have zip iterators that could be used to implement this, I believe.
> > > > (see STLExtras.h)
> > > >
> > > > Alternatively, we could tail-allocate the Association objects with
> > > > (perhaps references to) pointers back into the Expr tail-allocated
> > > > array. Not ideal, but does provide a clean interface.
> > > >
> > > > @riccibruno may have other ideas on how to pack the arrays, as he's
> > > > done a lot of this work recently.
> > > > We have zip iterators that could be used to implement this, I believe.
> > >
> > > You're right, there is a `concat` there, but on second thought - because
> > > Association and Stmt don't share a base, I don't think it can work.
> > >
> > > > Alternatively, we could tail-allocate the Association objects with
> > > > (perhaps references to) pointers back into the Expr tail-allocated
> > > > array. Not ideal, but does provide a clean interface.
> > >
> > > Perhaps this can work, but I don't know how to do it. If you have scope
> > > for it in your part of the efforts, it would be a good way to get this
> > > unblocked.
> > > Perhaps this can work, but I don't know how to do it. If you have scope
> > > for it in your part of the efforts, it would be a good way to get this
> > > unblocked.
> >
> > I'll put some time into it today and see where it goes. You may be right
> > that this is more work than it's worth, so we'll see.
> I don't see what would prevent tail-allocating the array of sub-expression
> and the array of `TypeSourceInfo`, and removing the `getAssocExpr`,
> `getAssocTypeSourceInfo`, `getAssocType` interface in favor of a single
> `getAssociation`. Then create a range version of `getAssociation` using the
> fact that if you know where you are in the sub-expression array, you know
> where is the corresponding `TypeSourceInfo`. To know which index correspond
> to the selected sub-expression you could use one of the low bits in the
> `TypeSourceInfo` pointers.
>
> This means that `children` is still simple to implement, and users can use a
> single unified
> interface via `getAssociation` and the range version `associations()`. From
> the point of view of the users it would be like if the `Association` objects
> were stored contiguously.
Made a patch which roughly the above idea: D57098
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56959/new/
https://reviews.llvm.org/D56959
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
