[PATCH] D57098: [WIP][AST] NFC: Introduce new class GenericSelectionExpr::Association

[Aaron Ballman via Phabricator via cfe-commits]

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