[PATCH] D56959: [AST] NFC: Introduce new class GenericSelectionExpr::Association

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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],
----------------
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56959