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

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked an inline comment as done.
mibintc added inline comments.


================
Comment at: clang/include/clang/AST/Expr.h:3465
+    SubExprs[RHS] = rhs;
+    hasFPFeatures = true;
+    isCompoundAssign = 1;
----------------
rjmccall wrote:
> 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() { ... }
> ```
The reason I set hasFPFeatures to True in this revision is because in the previous review you asked me to make it purely a representational change and "add stuff about whether there is a pragma in a separate patch".  The stuff I had in the previous version was smarter about creating trailing storage, it only created trailing storage when the pragma was in effect. I envirsioned that the evolution would accept something that always created the FPOptions in trailing storage, and in the 2nd generation would adopt the more thrifty way of creating trailing storage. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76384