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

[steveire]

Summary

As a type, this can be visited upon. This is similar in spirit to the
BlockDecl::Capture type.

[riccibruno]

include/clang/AST/Expr.h

5020

I know that this is not part of this patch, but these arrays are begging
to be tail-allocated if you want to have a go at it.

5047

What about some comments ? I can guess but it is better to have
too much doc than not enough imho.

[aaron.ballman]

include/clang/AST/Expr.h

5068

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).

lib/AST/ASTDumper.cpp

1467

Don't use auto here.

lib/AST/StmtPrinter.cpp

1266

Don't use auto.

lib/AST/StmtProfile.cpp

1263

Or here (or anywhere similar in this patch; I'll stop commenting on them).

[steveire]

include/clang/AST/Expr.h

5068

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.

[aaron.ballman]

include/clang/AST/Expr.h

5068

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.

[steveire]

include/clang/AST/Expr.h

5068

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.

[aaron.ballman]

include/clang/AST/Expr.h

5068

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.

[riccibruno]

include/clang/AST/Expr.h

5068

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.

[riccibruno]

Some small additional remarks if you are already modifying this class.

include/clang/AST/Expr.h

5021

It is possible to stuff one SourceLocation in the bit-fields of Stmt to save one more pointer.

5113

I believe these LLVM_READONLY are pointless here.

5125

Move this friend decl to the top ?

[riccibruno]

include/clang/AST/Expr.h

5068

Made a patch which roughly the above idea: D57098

[steveire]

An alternative was written and committed.

include/clang/AST/Expr.h

 /// Any generic selection whose controlling expression is type-dependent or
 /// which names a dependent type in its association list is result-dependent,
 /// which means that the choice of result expression is dependent.
 /// Result-dependent generic associations are both type- and value-dependent.
 class GenericSelectionExpr : public Expr {
   enum { CONTROLLING, END_EXPR };
   TypeSourceInfo **AssocTypes;
   Stmt **SubExprs;
   unsigned NumAssocs, ResultIndex;
   SourceLocation GenericLoc, DefaultLoc, RParenLoc;
 
 public:
   GenericSelectionExpr(const ASTContext &Context,
                        SourceLocation GenericLoc, Expr *ControllingExpr,
                        ArrayRef<TypeSourceInfo*> AssocTypes,
                        ArrayRef<Expr*> AssocExprs,
                        SourceLocation DefaultLoc, SourceLocation RParenLoc,
                        bool ContainsUnexpandedParameterPack,
                        unsigned ResultIndex);
 
   /// This constructor is used in the result-dependent case.
   GenericSelectionExpr(const ASTContext &Context,
                        SourceLocation GenericLoc, Expr *ControllingExpr,
                        ArrayRef<TypeSourceInfo*> AssocTypes,
                        ArrayRef<Expr*> AssocExprs,
                        SourceLocation DefaultLoc, SourceLocation RParenLoc,
                        bool ContainsUnexpandedParameterPack);
 
   explicit GenericSelectionExpr(EmptyShell Empty)
     : Expr(GenericSelectionExprClass, Empty) { }
 
+  class Association {
+    Expr *E;
+    TypeSourceInfo *TSI;
+    bool Selected;
+
+  public:
+    Association(Expr *E, TypeSourceInfo *TSI, bool Selected)
+        : E(E), TSI(TSI), Selected(Selected) {}
+
+    Expr *getExpr() { return E; }
+    const Expr *getExpr() const { return E; }
+    TypeSourceInfo *getTypeSourceInfo() { return TSI; }
+    const TypeSourceInfo *getTypeSourceInfo() const { return TSI; }
+
+    QualType getType() const {
+      if (TSI)
+        return TSI->getType();
+      return QualType();
+    }
+
+    bool IsSelected() const { return Selected; }
+  };
+
   unsigned getNumAssocs() const { return NumAssocs; }
 
+  Association getAssociation(unsigned I) const {
+    return Association(cast<Expr>(SubExprs[END_EXPR + I]), AssocTypes[I],
+                       !isResultDependent() && getResultIndex() == I);
+  }
+
   SourceLocation getGenericLoc() const { return GenericLoc; }
   SourceLocation getDefaultLoc() const { return DefaultLoc; }
   SourceLocation getRParenLoc() const { return RParenLoc; }
 
-  const Expr *getAssocExpr(unsigned i) const {
-    return cast<Expr>(SubExprs[END_EXPR+i]);
-  }
-  Expr *getAssocExpr(unsigned i) { return cast<Expr>(SubExprs[END_EXPR+i]); }
   ArrayRef<Expr *> getAssocExprs() const {
     return NumAssocs
                ? llvm::makeArrayRef(
                      &reinterpret_cast<Expr **>(SubExprs)[END_EXPR], NumAssocs)
                : None;
   }
-  const TypeSourceInfo *getAssocTypeSourceInfo(unsigned i) const {
-    return AssocTypes[i];
-  }
-  TypeSourceInfo *getAssocTypeSourceInfo(unsigned i) { return AssocTypes[i]; }
   ArrayRef<TypeSourceInfo *> getAssocTypeSourceInfos() const {
     return NumAssocs ? llvm::makeArrayRef(&AssocTypes[0], NumAssocs) : None;
   }
 
-  QualType getAssocType(unsigned i) const {
-    if (const TypeSourceInfo *TS = getAssocTypeSourceInfo(i))
-      return TS->getType();
-    else
-      return QualType();
-  }
-
   const Expr *getControllingExpr() const {
     return cast<Expr>(SubExprs[CONTROLLING]);
   }
   Expr *getControllingExpr() { return cast<Expr>(SubExprs[CONTROLLING]); }
 
   /// Whether this generic selection is result-dependent.
   bool isResultDependent() const { return ResultIndex == -1U; }
 
   /// The zero-based index of the result expression's generic association in
   /// the generic selection's association list.  Defined only if the
   /// generic selection is not result-dependent.
   unsigned getResultIndex() const {
     assert(!isResultDependent() && "Generic selection is result-dependent");
     return ResultIndex;
   }
 
   /// The generic selection's result expression.  Defined only if the
   /// generic selection is not result-dependent.
-  const Expr *getResultExpr() const { return getAssocExpr(getResultIndex()); }
-  Expr *getResultExpr() { return getAssocExpr(getResultIndex()); }
+  const Expr *getResultExpr() const {
+    return cast<Expr>(SubExprs[END_EXPR + getResultIndex()]);
+  }
+  Expr *getResultExpr() {
+    return cast<Expr>(SubExprs[END_EXPR + getResultIndex()]);
+  }
 
   SourceLocation getBeginLoc() const LLVM_READONLY { return GenericLoc; }
   SourceLocation getEndLoc() const LLVM_READONLY { return RParenLoc; }
 
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == GenericSelectionExprClass;
   }
 
   child_range children() {
     return child_range(SubExprs, SubExprs+END_EXPR+NumAssocs);
   }
   const_child_range children() const {
     return const_child_range(SubExprs, SubExprs + END_EXPR + NumAssocs);
   }
   friend class ASTStmtReader;
 };
 
 //===----------------------------------------------------------------------===//
 // Clang Extensions
 //===----------------------------------------------------------------------===//
 
 /// ExtVectorElementExpr - This represents access to specific elements of a
 /// vector, and may occur on the left hand side or right hand side.  For example

include/clang/AST/RecursiveASTVisitor.h

 }
 
 // GenericSelectionExpr is a special case because the types and expressions
 // are interleaved.  We also need to watch out for null types (default
 // generic associations).
 DEF_TRAVERSE_STMT(GenericSelectionExpr, {
   TRY_TO(TraverseStmt(S->getControllingExpr()));
   for (unsigned i = 0; i != S->getNumAssocs(); ++i) {
-    if (TypeSourceInfo *TS = S->getAssocTypeSourceInfo(i))
+    auto Assoc = S->getAssociation(i);
+    if (const TypeSourceInfo *TS = Assoc.getTypeSourceInfo())
       TRY_TO(TraverseTypeLoc(TS->getTypeLoc()));
-    TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(S->getAssocExpr(i));
+    TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(Assoc.getExpr());
   }
   ShouldVisitChildren = false;
 })
 
 // PseudoObjectExpr is a special case because of the weirdness with
 // syntactic expressions and opaque values.
 DEF_TRAVERSE_STMT(PseudoObjectExpr, {
   TRY_TO_TRAVERSE_OR_ENQUEUE_STMT(S->getSyntacticForm());

include/clang/AST/StmtDataCollectors.td

 class CXXFoldExpr {
   code Code = [{
     addData(S->isRightFold()); addData(S->getOperator());
   }];
 }
 class GenericSelectionExpr {
   code Code = [{
     for (unsigned i = 0; i < S->getNumAssocs(); ++i) {
-      addData(S->getAssocType(i));
+      addData(S->getAssociation(i).getType());
     }
   }];
 }
 class LambdaExpr {
   code Code = [{
     for (const LambdaCapture &C : S->captures()) {
       addData(C.isPackExpansion());
       addData(C.getCaptureKind());

lib/AST/ASTDumper.cpp

 void ASTDumper::VisitGenericSelectionExpr(const GenericSelectionExpr *E) {
   if (E->isResultDependent())
     OS << " result_dependent";
   dumpStmt(E->getControllingExpr());
   dumpTypeAsChild(E->getControllingExpr()->getType()); // FIXME: remove
 
   for (unsigned I = 0, N = E->getNumAssocs(); I != N; ++I) {
     dumpChild([=] {
-      if (const TypeSourceInfo *TSI = E->getAssocTypeSourceInfo(I)) {
+      const auto Assoc = E->getAssociation(I);
+      const TypeSourceInfo *TSI = Assoc.getTypeSourceInfo();
+      if (TSI) {
         OS << "case ";
         NodeDumper.dumpType(TSI->getType());
       } else {
         OS << "default";
       }
 
-      if (!E->isResultDependent() && E->getResultIndex() == I)
+      if (Assoc.IsSelected())
         OS << " selected";
 
-      if (const TypeSourceInfo *TSI = E->getAssocTypeSourceInfo(I))
+      if (TSI)
         dumpTypeAsChild(TSI->getType());
-      dumpStmt(E->getAssocExpr(I));
+      dumpStmt(Assoc.getExpr());
     });
   }
 }
 
 //===----------------------------------------------------------------------===//
 // C++ Expressions
 //===----------------------------------------------------------------------===//
 

lib/AST/StmtPrinter.cpp

void StmtPrinter::VisitUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *Node){
   }
 }
 
 void StmtPrinter::VisitGenericSelectionExpr(GenericSelectionExpr *Node) {
   OS << "_Generic(";
   PrintExpr(Node->getControllingExpr());
   for (unsigned i = 0; i != Node->getNumAssocs(); ++i) {
     OS << ", ";
-    QualType T = Node->getAssocType(i);
+    auto Assoc = Node->getAssociation(i);
+    QualType T = Assoc.getType();
     if (T.isNull())
       OS << "default";
     else
       T.print(OS, Policy);
     OS << ": ";
-    PrintExpr(Node->getAssocExpr(i));
+    PrintExpr(Assoc.getExpr());
   }
   OS << ")";
 }
 
 void StmtPrinter::VisitArraySubscriptExpr(ArraySubscriptExpr *Node) {
   PrintExpr(Node->getLHS());
   OS << "[";
   PrintExpr(Node->getRHS());

lib/AST/StmtProfile.cpp

 void StmtProfiler::VisitBlockExpr(const BlockExpr *S) {
   VisitExpr(S);
   VisitDecl(S->getBlockDecl());
 }
 
 void StmtProfiler::VisitGenericSelectionExpr(const GenericSelectionExpr *S) {
   VisitExpr(S);
   for (unsigned i = 0; i != S->getNumAssocs(); ++i) {
-    QualType T = S->getAssocType(i);
+    auto Assoc = S->getAssociation(i);
+    QualType T = Assoc.getType();
     if (T.isNull())
       ID.AddPointer(nullptr);
     else
       VisitType(T);
-    VisitExpr(S->getAssocExpr(i));
+    VisitExpr(Assoc.getExpr());
   }
 }
 
 void StmtProfiler::VisitPseudoObjectExpr(const PseudoObjectExpr *S) {
   VisitExpr(S);
   for (PseudoObjectExpr::const_semantics_iterator
          i = S->semantics_begin(), e = S->semantics_end(); i != e; ++i)
     // Normally, we would not profile the source expressions of OVEs.

lib/Sema/SemaExprObjC.cpp

return new (Context)
                       sub->getObjectKind(), uo->getOperatorLoc(), false);
   } else if (GenericSelectionExpr *gse = dyn_cast<GenericSelectionExpr>(e)) {
     assert(!gse->isResultDependent());
 
     unsigned n = gse->getNumAssocs();
     SmallVector<Expr*, 4> subExprs(n);
     SmallVector<TypeSourceInfo*, 4> subTypes(n);
     for (unsigned i = 0; i != n; ++i) {
-      subTypes[i] = gse->getAssocTypeSourceInfo(i);
-      Expr *sub = gse->getAssocExpr(i);
+      auto Assoc = gse->getAssociation(i);
+      subTypes[i] = Assoc.getTypeSourceInfo();
+      Expr *sub = Assoc.getExpr();
       if (i == gse->getResultIndex())
         sub = stripARCUnbridgedCast(sub);
       subExprs[i] = sub;
     }
 
     return new (Context) GenericSelectionExpr(Context, gse->getGenericLoc(),
                                               gse->getControllingExpr(),
                                               subTypes, subExprs,

lib/Sema/SemaPseudoObject.cpp

Expr *rebuild(Expr *e) {
         assert(!gse->isResultDependent());
         unsigned resultIndex = gse->getResultIndex();
         unsigned numAssocs = gse->getNumAssocs();
 
         SmallVector<Expr*, 8> assocs(numAssocs);
         SmallVector<TypeSourceInfo*, 8> assocTypes(numAssocs);
 
         for (unsigned i = 0; i != numAssocs; ++i) {
-          Expr *assoc = gse->getAssocExpr(i);
+          auto A = gse->getAssociation(i);
+          Expr *assoc = A.getExpr();
           if (i == resultIndex) assoc = rebuild(assoc);
           assocs[i] = assoc;
-          assocTypes[i] = gse->getAssocTypeSourceInfo(i);
+          assocTypes[i] = A.getTypeSourceInfo();
         }
 
         return new (S.Context) GenericSelectionExpr(S.Context,
                                                     gse->getGenericLoc(),
                                                     gse->getControllingExpr(),
                                                     assocTypes,
                                                     assocs,
                                                     gse->getDefaultLoc(),

lib/Sema/TreeTransform.h

TreeTransform<Derived>::TransformGenericSelectionExpr(GenericSelectionExpr *E) {
   ExprResult ControllingExpr =
     getDerived().TransformExpr(E->getControllingExpr());
   if (ControllingExpr.isInvalid())
     return ExprError();
 
   SmallVector<Expr *, 4> AssocExprs;
   SmallVector<TypeSourceInfo *, 4> AssocTypes;
   for (unsigned i = 0; i != E->getNumAssocs(); ++i) {
-    TypeSourceInfo *TS = E->getAssocTypeSourceInfo(i);
+    auto Assoc = E->getAssociation(i);
+    TypeSourceInfo *TS = Assoc.getTypeSourceInfo();
     if (TS) {
       TypeSourceInfo *AssocType = getDerived().TransformType(TS);
       if (!AssocType)
         return ExprError();
       AssocTypes.push_back(AssocType);
     } else {
       AssocTypes.push_back(nullptr);
     }
 
-    ExprResult AssocExpr = getDerived().TransformExpr(E->getAssocExpr(i));
+    ExprResult AssocExpr = getDerived().TransformExpr(Assoc.getExpr());
     if (AssocExpr.isInvalid())
       return ExprError();
     AssocExprs.push_back(AssocExpr.get());
   }
 
   return getDerived().RebuildGenericSelectionExpr(E->getGenericLoc(),
                                                   E->getDefaultLoc(),
                                                   E->getRParenLoc(),

lib/Serialization/ASTWriterStmt.cpp

 }
 
 void ASTStmtWriter::VisitGenericSelectionExpr(GenericSelectionExpr *E) {
   VisitExpr(E);
   Record.push_back(E->getNumAssocs());
 
   Record.AddStmt(E->getControllingExpr());
   for (unsigned I = 0, N = E->getNumAssocs(); I != N; ++I) {
-    Record.AddTypeSourceInfo(E->getAssocTypeSourceInfo(I));
-    Record.AddStmt(E->getAssocExpr(I));
+    auto Assoc = E->getAssociation(I);
+    Record.AddTypeSourceInfo(Assoc.getTypeSourceInfo());
+    Record.AddStmt(Assoc.getExpr());
   }
   Record.push_back(E->isResultDependent() ? -1U : E->getResultIndex());
 
   Record.AddSourceLocation(E->getGenericLoc());
   Record.AddSourceLocation(E->getDefaultLoc());
   Record.AddSourceLocation(E->getRParenLoc());
   Code = serialization::EXPR_GENERIC_SELECTION;
 }