aaron.ballman added a comment.
This is looking great! I've got some comments, mostly nits. Just to
double-check, have you verified that these changes do not break any existing
tests (in clang or clang-tools-extra)?
================
Comment at: include/clang/AST/Expr.h:5026
+ /// result expression in the case where the generic selection expression
+ /// is not result-dependent. The result index is equal to -1u if and only
+ /// if the generic selection expression is result-dependent.
----------------
`~0U` instead of `-1u` so it doesn't suggest a weird type mismatch?
================
Comment at: include/clang/AST/Expr.h:5030
-public:
- GenericSelectionExpr(const ASTContext &Context,
- SourceLocation GenericLoc, Expr *ControllingExpr,
- ArrayRef<TypeSourceInfo*> AssocTypes,
- ArrayRef<Expr*> AssocExprs,
- SourceLocation DefaultLoc, SourceLocation RParenLoc,
+ /// The location of the "default" and of the right parenthese.
+ SourceLocation DefaultLoc, RParenLoc;
----------------
parenthese -> parenthesis
================
Comment at: include/clang/AST/Expr.h:5034
+ // GenericSelectionExpr is followed by several trailing objects.
+ // They are in order:
+ //
----------------
They are (in order):
================
Comment at: include/clang/AST/Expr.h:5039
+ // * An array of getNumAssocs() TypeSourceInfo *, one for each of the
+ // association expression.
+ enum { CONTROLLING = 0, ASSOC_EXPR_START = 1 };
----------------
expression -> expressions
================
Comment at: include/clang/AST/Expr.h:5043
+ unsigned numTrailingObjects(OverloadToken<Stmt *>) const {
+ return 1 + getNumAssocs();
+ }
----------------
I think it would be good to put a comment here like "Add one to account for the
controlling expression; the remainder are the associated expressions."
================
Comment at: include/clang/AST/Expr.h:5094
+ public:
+ using iterator_category = std::forward_iterator_tag;
+ using value_type = AssociationTy<Const>;
----------------
It seems like this should be pretty trivial to make into a random access
iterator, or am I missing something?
================
Comment at: include/clang/AST/Expr.h:5097
+ using difference_type = std::ptrdiff_t;
+ using pointer = AssociationTy<Const>;
+ using reference = AssociationTy<Const>;
----------------
Cute, but I suspect this may come back to bite us at some point. For instance,
if someone thinks they're working with a real pointer, they're likely to expect
pointer arithmetic to work when it won't (at least they'll get compile errors
though).
================
Comment at: include/clang/AST/Expr.h:5101
+ AssociationTy<Const> operator*() const {
+ return AssociationTy<Const>(cast<Expr>(*E), *TSI,
+ Offset == SelectedOffset);
----------------
This should return `reference` instead.
================
Comment at: include/clang/AST/Expr.h:5104
+ }
+ AssociationTy<Const> operator->() const { return **this; }
+ AssociationIteratorTy &operator++() {
----------------
This should return `pointer` instead.
================
Comment at: include/clang/AST/Expr.h:5106-5108
+ E += 1;
+ TSI += 1;
+ Offset += 1;
----------------
Use `++Foo` for each of these?
================
Comment at: include/clang/AST/Expr.h:5186
+ /// Whether this generic selection is result-dependent.
+ bool isResultDependent() const { return ResultIndex == -1U; }
----------------
`std::numeric_limits<unsigned>::max()`?
================
Comment at: include/clang/AST/Expr.h:5208
+ ArrayRef<Expr *> getAssocExprs() const {
+ return {reinterpret_cast<Expr *const *>(getTrailingObjects<Stmt *>() +
+ ASSOC_EXPR_START),
----------------
Do we need to use `reinterpret_cast` here, or can this be done through llvm's
casting machinery instead?
================
Comment at: include/clang/AST/Expr.h:5219
+ Association getAssociation(unsigned I) {
+ assert((I < getNumAssocs()) &&
+ "Out-of-range index in GenericSelectionExpr::getAssociation!");
----------------
Spurious parens can be removed.
================
Comment at: lib/AST/Expr.cpp:3791
+ DefaultLoc(DefaultLoc), RParenLoc(RParenLoc) {
+ assert((AssocTypes.size() == AssocExprs.size()) &&
+ "Must have the same number of association expressions"
----------------
Spurious parens.
================
Comment at: lib/AST/Expr.cpp:3794
+ " and TypeSourceInfo!");
+ assert((ResultIndex < NumAssocs) && "ResultIndex is out-of-bounds!");
+
----------------
Spurious parens.
================
Comment at: lib/AST/Expr.cpp:3816
+ RParenLoc(RParenLoc) {
+ assert((AssocTypes.size() == AssocExprs.size()) &&
+ "Must have the same number of association expressions"
----------------
Spurious parens.
================
Comment at: lib/Sema/SemaExprObjC.cpp:4339
+ subTypes.reserve(n);
+ for (GenericSelectionExpr::Association Assoc : gse->associations()) {
+ subTypes.push_back(Assoc.getTypeSourceInfo());
----------------
This should probably be spelled `assoc` to be consistent with the surrounding
code.
================
Comment at: lib/Sema/SemaPseudoObject.cpp:148
+
+ for (GenericSelectionExpr::Association Assoc : gse->associations()) {
+ Expr *assoc = Assoc.getAssociationExpr();
----------------
Probably have to go with `assoc` here.
================
Comment at: lib/Sema/SemaPseudoObject.cpp:149
+ for (GenericSelectionExpr::Association Assoc : gse->associations()) {
+ Expr *assoc = Assoc.getAssociationExpr();
+ if (Assoc.isSelected())
----------------
This should be renamed regardless; probably `assocExpr`
================
Comment at: lib/Serialization/ASTReaderStmt.cpp:1027
+ unsigned NumAssocs = Record.readInt();
+ assert((NumAssocs == E->getNumAssocs()) && "Wrong NumAssocs!");
+ E->ResultIndex = Record.readInt();
----------------
Spurious parens.
================
Comment at: lib/Serialization/ASTReaderStmt.cpp:1040
+ TSIs[I] = GetTypeSourceInfo();
+ ;
}
----------------
Spurious semi-colon.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57098/new/
https://reviews.llvm.org/D57098
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
