[PATCH] D140584: [Clang] Refactor "Designators" into a unified implementation [NFC]

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/AST/Designator.h:87-92
+    /// Refers to the index expression.
+    Expr *IndexExpr;
+
+    /// Location of the first index expression within the designated
+    /// initializer expression's list of subexpressions.
+    unsigned Index;
----------------
This looks very odd -- these two index fields represent completely different kinds of information, one of which is specific to an implementation detail of `InitListExpr` / `DesignatedInitExpr`. Perhaps this class could be templated over its representation of an expression, instead of having both cases be possibilities here, and having the user of this class keep track of whether they're using the kind of designator that stores an expression pointer or the kind that stores an expression index? (We'd end up storing two different indexes for array range designators this way, but that seems like it should not be problematic.)


================
Comment at: clang/include/clang/AST/Designator.h:98
+    /// The location of the ']' terminating the array designator.
+    mutable SourceLocation RBracketLoc;
+
----------------
Does this really need to be mutable? The AST version shouldn't be exposing a way to mutate this field! It seems like we should have a non-const overload of `Designation::getDesignator` for `Sema` to use if it really needs to modify this field.


================
Comment at: clang/include/clang/AST/Designator.h:313-319
+  /// ClearExprs - Null out any expression references, which prevents
+  /// them from being 'delete'd later.
+  void ClearExprs(Sema &Actions) {}
+
+  /// FreeExprs - Release any unclaimed memory for the expressions in
+  /// this designator.
+  void FreeExprs(Sema &Actions) {}
----------------
AST classes shouldn't reference `Sema`. But these functions do nothing; can they be removed?


================
Comment at: clang/include/clang/AST/Designator.h:340-346
+  /// ClearExprs - Null out any expression references, which prevents them from
+  /// being 'delete'd later.
+  void ClearExprs(Sema &Actions) {}
+
+  /// FreeExprs - Release any unclaimed memory for the expressions in this
+  /// designation.
+  void FreeExprs(Sema &Actions) {}
----------------
AST classes shouldn't reference `Sema`. But these functions do nothing; can they be removed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140584