rjmccall added a comment.
Thanks. Just a bunch of fairly minor requests now.
================
Comment at: clang/include/clang/AST/Expr.h:3474
+ size_t offsetOfTrailingStorage() const;
+ size_t offsetOfTrailingStorage();
+
----------------
You don't need the non-const overload.
================
Comment at: clang/include/clang/AST/Expr.h:3719
+ /// Used to allocate the right amount of storage.
+ static unsigned sizeOfTrailingObjects(unsigned HasFPFeatures) {
+ return HasFPFeatures * sizeof(FPOptions);
----------------
Should `HasFPFeatures` be a `bool` everywhere you're passing it around?
================
Comment at: clang/lib/AST/Expr.cpp:4397
+ : sizeof(BinaryOperator);
+}
+
----------------
Please define this in the header. You can just do:
```
inline size_t BinaryOperator::offsetOfTrailingStorage() const { ... }
```
after both of the classes are fully defined.
================
Comment at: clang/lib/Analysis/BodyFarm.cpp:120
+ VK_RValue, OK_Ordinary, SourceLocation(),
+ FPOptions::defaultWithoutTrailingStorage(C));
}
----------------
These construction sites don't seem like appropriate uses of
`defaultWithoutTrailingStorage`, which is an implementation detail of the AST
nodes. The right behavior here is for all of these places to use the default
`FPOptions` from the language options, then let the AST figure out the right
way to store that. That happens to have the same effect currently as
`defaultWithoutTrailingStorage`, but the latter should be able to change, while
the right behavior for these places will remain the same.
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
