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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Trying to help Melanie respond to the comments, so I ran through the patch and came up with the following comments/responses.  Sorry for the 'surprise hit' :)



================
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);
----------------
rjmccall wrote:
> 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.
Yep, this shouldn't be necessary.  Uses of this should be able to use totalSizeToAlloc and additionalSizeToAlloc.


================
Comment at: clang/include/clang/AST/Expr.h:3674
+
+  void setHasFPFeatures(bool B) { BinaryOperatorBits.HasFPFeatures = B; }
+  bool hasFPFeatures() const { return BinaryOperatorBits.HasFPFeatures; }
----------------
Since changing this value can result in a change of allocation size, I don't think this should be settable after creation.


================
Comment at: clang/include/clang/AST/Expr.h:3680
+  }
+  FPOptions getFPFeatures(const ASTContext &C) const {
+    if (hasFPFeatures())
----------------
Whats the purpose of having both of these?  It seems that you can either: 1- Have only the first and require consumers to check if they aren't sure, or 2- Only do the second, and those who have checked just get the default anyway.


================
Comment at: clang/include/clang/AST/Expr.h:3739
 
-  BinaryOperator(StmtClass SC, EmptyShell Empty) : Expr(SC, Empty) {
-    BinaryOperatorBits.Opc = BO_MulAssign;
-  }
-};
-
-/// CompoundAssignOperator - For compound assignments (e.g. +=), we keep
-/// track of the type the operation is performed in.  Due to the semantics of
-/// these operators, the operands are promoted, the arithmetic performed, an
-/// implicit conversion back to the result type done, then the assignment takes
-/// place.  This captures the intermediate type which the computation is done
-/// in.
-class CompoundAssignOperator : public BinaryOperator {
-  QualType ComputationLHSType;
-  QualType ComputationResultType;
-public:
-  CompoundAssignOperator(Expr *lhs, Expr *rhs, Opcode opc, QualType ResType,
-                         ExprValueKind VK, ExprObjectKind OK,
-                         QualType CompLHSType, QualType CompResultType,
-                         SourceLocation OpLoc, FPOptions FPFeatures)
-    : BinaryOperator(lhs, rhs, opc, ResType, VK, OK, OpLoc, FPFeatures,
-                     true),
-      ComputationLHSType(CompLHSType),
-      ComputationResultType(CompResultType) {
-    assert(isCompoundAssignmentOp() &&
-           "Only should be used for compound assignments");
-  }
-
-  /// Build an empty compound assignment operator expression.
-  explicit CompoundAssignOperator(EmptyShell Empty)
-    : BinaryOperator(CompoundAssignOperatorClass, Empty) { }
-
-  // The two computation types are the type the LHS is converted
-  // to for the computation and the type of the result; the two are
-  // distinct in a few cases (specifically, int+=ptr and ptr-=ptr).
-  QualType getComputationLHSType() const { return ComputationLHSType; }
-  void setComputationLHSType(QualType T) { ComputationLHSType = T; }
-
-  QualType getComputationResultType() const { return ComputationResultType; }
-  void setComputationResultType(QualType T) { ComputationResultType = T; }
-
-  static bool classof(const Stmt *S) {
-    return S->getStmtClass() == CompoundAssignOperatorClass;
+  BinaryOperator(EmptyShell Empty, unsigned hasFPFeatures, unsigned isCompound)
+      : Expr(BinaryOperatorClass, Empty) {
----------------
Nit, I'd prefer this be named 'isCompoundAssignment'.  Also, the patch seems a little inconsistent with FPFeatures vs FPOptions.


================
Comment at: clang/include/clang/AST/StmtVisitor.h:146
+  RetTy VisitBin##NAME(PTR(BinaryOperator) S, ParamTys... P) {                 \
+    DISPATCH(BinAssign, BinaryOperator);                                       \
   }
----------------
rjmccall wrote:
> Comment needs updating, but more importantly, you're making all of these fall back on `VisitBinAssign`, which is definitely not correct.
@rjmccall : What would you think fits better?  If we don't have something like this, VisitBinaryOperator is going to have to redirect to VisitBinAssign anyway.  We could presumably keep VisitCompoundAssignOperator, but that seems like a naming difference.


================
Comment at: clang/include/clang/Basic/LangOptions.h:385
+  static FPOptions defaultWithoutTrailingStorage() {
+    FPOptions result;
+    return result;
----------------
Nit: return {}; ?  

Also, why are there two versions of this?  It seems that the ASTContext one should be the only required one.


================
Comment at: clang/include/clang/Basic/LangOptions.h:394
+     return true;
+  }
+
----------------
rjmccall wrote:
> 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..
I think I disagree with @rjmccall that these requiresTrailingStorage should be here at all.  I think we should store in the AST ANY programmer opinion, even if they match the global setting.  It seems to me that this would be more tolerant of any global-setting rewrites that modules/et-al introduce, as well as make the AST Print consistent.  Always storing FPOptions when the user has explicitly overriding it also better captures the programmer's intent.


================
Comment at: clang/lib/AST/Expr.cpp:4347
+  unsigned SizeOfTrailingObjects =
+      BinaryOperator::sizeOfTrailingObjects(hasFPFeatures, isCompound);
+  void *Mem = C.Allocate(sizeof(BinaryOperator) + SizeOfTrailingObjects,
----------------
Again, should just use the TrailingObjects::totalSizeToAlloc I think.  Alternatively, implement a new 'totalSizeToAlloc' based on the hasFPFeatures and isCompound).


================
Comment at: clang/lib/AST/ExprConstant.cpp:7788
 
-bool LValueExprEvaluator::VisitCompoundAssignOperator(
-    const CompoundAssignOperator *CAO) {
+bool LValueExprEvaluator::VisitBinAssign(const BinaryOperator *E) {
   if (!Info.getLangOpts().CPlusPlus14 && !Info.keepEvaluatingAfterFailure())
----------------
Perhaps not changing this name is a good idea, and just leaving this as VisitCompoundOperator, then having the 'default' behavior to be to just call VisitBinaryOperator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76384