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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
