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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
