[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

John McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Mar 25 09:10:36 PDT 2020


rjmccall added inline comments.


================
Comment at: clang/include/clang/AST/Expr.h:2573
 
+  FPOptions FPFeatures;
+
----------------
Let's split the CallExpr changes out into a separate patch, so that we have one patch that's *purely* about unifying BinaryOperator and CompoundAssignOperator and putting FPOptions in trailing storage, and then another that's about adding FPOptions to CallExpr.

For that latter patch, CallExpr already has its own, hand-rolled concept of trailing storage which you should be able to emulate instead of unifying the hierarchy.


================
Comment at: clang/include/clang/AST/Expr.h:3417
+    : public Expr,
+      private llvm::TrailingObjects<BinaryOperator, unsigned, QualType> {
   enum { LHS, RHS, END_EXPR };
----------------
Why does this use `unsigned` for the trailing storage of the `FPOptions`?


================
Comment at: clang/include/clang/AST/Expr.h:3421
+  bool hasFPFeatures;
+  bool isCompoundAssign;
+
----------------
Can these bits go in the bitfields?


================
Comment at: clang/include/clang/AST/Expr.h:3465
+    SubExprs[RHS] = rhs;
+    hasFPFeatures = true;
+    isCompoundAssign = 1;
----------------
Okay, so this is *always* adding trailing storage.  Is there a common value for `FPOptions` — something that corresponds to default settings, for example — that we could use to avoid needing storage in the common case?

Also, it would be useful to extract a function somewhere that you can use in all of these places for consistency, maybe something like this on `FPOptions`:

```
  /// Does this FPOptions require trailing storage when stored in various AST nodes,
  /// or can it be recreated using `defaultWithoutTrailingStorage`?
  bool requiresTrailingStorage() const { return *this == defaultWithoutTrailingStorage(); }

  /// Return the default value of FPOptions that's used when trailing storage isn't required.
  static FPOptions defaultWithoutTrailingStorage() { ... }
```


================
Comment at: clang/include/clang/AST/Expr.h:3471
+    assert(isCompoundAssignmentOp() &&
+           "Expected CompoundAssignOperator for compound assignments");
+    setComputationLHSType(CompLHSType);
----------------
Please change the text in this assertion and the one in the constructor above.  The requirement is now that you use the appropriate constructor for the case.  Or maybe we should just have one constructor that takes optional CompLHS/CompResult types and then assert that they're given if we're building a compound assignment?  If you do the same for `Create`, the whole thing might end up being much more convenient for callers, too.


================
Comment at: clang/include/clang/AST/Expr.h:3643
+    return *getTrailingObjects<unsigned>();
+  }
+
----------------
Are these necessary?


================
Comment at: clang/include/clang/AST/Expr.h:3676
+    getTrailingObjects<QualType>()[1] = T;
+  }
+
----------------
These should all assert that they're being used on a compound-assignment operator.

Please preserve the comments from CompoundAssignOperator on these.


================
Comment at: clang/include/clang/AST/Expr.h:6029
   friend class ASTStmtWriter;
 };
 
----------------
Why is this in this patch?


================
Comment at: clang/lib/AST/Expr.cpp:4285
+                                       FPOptions FPFeatures) {
+  bool hasFPFeatures = true;
+  unsigned SizeOfTrailingObjects =
----------------
These places should use that same unified predicate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76384





More information about the cfe-commits mailing list