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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/AST/Expr.h:3474
+  static unsigned sizeOfTrailingObjects(bool hasFP, bool isCompound) {
+    return (hasFP ? 1 : 0) * sizeof(unsigned) +
+           (isCompound ? 2 : 0) * sizeof(QualType);
----------------
Sorry, I wasn't trying to say you need this here!  Since you can use TrailingObjects for BinaryOperator, you absolutely should take advantage of what it does.   I was just pointing you at the CallExpr stuff so that you can see the places you'll need to update when you add this storage to CallExpr.


================
Comment at: clang/include/clang/AST/Expr.h:3679
+    return *getTrailingObjects<FPOptions>();
+  }
+  FPOptions getFPFeatures(const ASTContext &C) const {
----------------
I'm not sure it's a good idea to have this accessor, at least not with such a tempting name.  Maybe `getStoredFPFeatures()` and `hasStoredFPFeatures()`?


================
Comment at: clang/include/clang/AST/Expr.h:3695
   // operations on floating point types.
   bool isFEnvAccessOn() const { return getFPFeatures().allowFEnvAccess(); }
 
----------------
These two accessors will need to take `ASTContext`s eventually.  Are you planning to do that in a follow-up patch?


================
Comment at: clang/include/clang/AST/JSONNodeDumper.h:265
   void VisitBinaryOperator(const BinaryOperator *BO);
-  void VisitCompoundAssignOperator(const CompoundAssignOperator *CAO);
+  void VisitCompoundAssignOperator(const BinaryOperator *CAO);
   void VisitMemberExpr(const MemberExpr *ME);
----------------
What actually calls this?  `StmtVisitor` as written doesn't fall back on a VisitCompoundAssignOperator.  You could *make* it fall back on one, of course.


================
Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:427
 // CompoundAssignOperator) but do have visitors.
-#define OPERATOR(NAME)                                                         \
-  GENERAL_BINOP_FALLBACK(NAME##Assign, CompoundAssignOperator)
+#define OPERATOR(NAME) GENERAL_BINOP_FALLBACK(NAME##Assign, BinaryOperator)
 
----------------
Comment needs updating.


================
Comment at: clang/include/clang/AST/StmtVisitor.h:146
+  RetTy VisitBin##NAME(PTR(BinaryOperator) S, ParamTys... P) {                 \
+    DISPATCH(BinAssign, BinaryOperator);                                       \
   }
----------------
Comment needs updating, but more importantly, you're making all of these fall back on `VisitBinAssign`, which is definitely not correct.


================
Comment at: clang/include/clang/Basic/LangOptions.h:394
+     return true;
+  }
+
----------------
The problem with having both functions that take `ASTContext`s and functions that don't is that it's easy to mix them, so they either need to have the same behavior (in which case it's pointless to have an overload that takes the `ASTContext`) or you're making something really error-prone.

I would feel a lot more confident that you were designing and using these APIs correctly if you actually took advantage of the ability to not store trailing FPOptions in some case, like when they match the global settings in the ASTContext.  That way you'll actually be verifying that everything behaves correctly if nodes don't store FPOptions.  If you do that, I think you'll see my point about not having all these easily-confusable functions that do or do not take `ASTContext`s..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76384