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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: clang/include/clang/AST/Expr.h:3465
+    SubExprs[RHS] = rhs;
+    hasFPFeatures = true;
+    isCompoundAssign = 1;
----------------
mibintc wrote:
> 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. 
Well, let's at least set up the infrastructure and APIs correctly, even if we always allocate trailing storage.

For example, should `getFPOptions` take an `ASTContext &` so that it can always return the right options instead of making clients do `hasFPOptions() ? getFPOptions() : ctx.getFPOptions()`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76384