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