[PATCH] D57104: [AST] Pack GenericSelectionExpr

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/AST/Expr.h:5043
+  //   association expressions.
+  enum { CONTROLLING = 0, ASSOC_EXPR_START = 1 };
+
----------------
Do we want to use `ControllingIndex` and `AssocExprStartIndex` and combine with the enum you introduced above?


================
Comment at: include/clang/AST/Expr.h:5048
+    // are the associated expressions.
+    return 1 + getNumAssocs();
+  }
----------------
riccibruno wrote:
> steveire wrote:
> > Would it be correct to use `ASSOC_EXPR_START` here instead of the magic `1`?
> Eh maybe ?
I think using this named constant adds confusion rather than clarifies things because it's not clear what the index has to do with the count. I think the comments suffice here rather than introducing a named constant with a sufficiently generic name.


================
Comment at: lib/AST/Expr.cpp:3814
+           /*isInstantiationDependent=*/true, ContainsUnexpandedParameterPack),
+      NumAssocs(AssocExprs.size()), ResultIndex(-1U), DefaultLoc(DefaultLoc),
+      RParenLoc(RParenLoc) {
----------------
`-1U` -> `ResultDependentIndex`


================
Comment at: lib/Serialization/ASTReaderStmt.cpp:1034
+  Stmt **Stmts = E->getTrailingObjects<Stmt *>();
+  for (unsigned I = 0, N = NumAssocs + 1; I < N; ++I)
+    Stmts[I] = Record.readSubExpr();
----------------
You should add a comment above the loop that explains the `+ 1` is for the controlling expression and that's why it's not needed for the type source information.


================
Comment at: lib/Serialization/ASTWriterStmt.cpp:979
+  Stmt **Stmts = E->getTrailingObjects<Stmt *>();
+  for (unsigned I = 0, N = E->getNumAssocs() + 1; I < N; ++I)
+    Record.AddStmt(Stmts[I]);
----------------
Similar comment here as above.


Repository:
  rC Clang

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

https://reviews.llvm.org/D57104