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

[John McCall via Phabricator via cfe-commits]

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