[PATCH] D81869: Modify FPFeatures to use delta not absolute settings to solve PCH compatibility problems

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Just a bunch of minor suggestions.  LGTM if you get all the tests worked out and it actually works the way you want on the tests.



================
Comment at: clang/include/clang/AST/Expr.h:2280
   }
+  FPOptionsOverride getFPFeatures(const LangOptions &LO) const {
+    if (UnaryOperatorBits.HasFPFeatures)
----------------
rjmccall wrote:
> I would call this one getFPOptionOverrides or getOverriddenFPOptions.  If you're going to have a getFPFeatures, it really ought to be the other one — although since we're changing pretty much all of the call sites, we might as well rename it to getFPOptions (or getFPOptionsInEffect, that seems fine, too).
I don't think there's any reason for this to take a LangOptions, since the set of overrides should be independent of the global settings.


================
Comment at: clang/include/clang/Basic/LangOptions.h:512
+    FPOptions result( (Base.getAsOpaqueInt() & ~OverrideMask)
+         | (OverrideMask & Options.getAsOpaqueInt()));
+    return result;
----------------
clang-format's suggestion is good here


================
Comment at: clang/include/clang/Basic/LangOptions.h:542
+  LLVM_DUMP_METHOD void dump();
+};
 
----------------
Same here: clang-format's just asking you to right-justify the escapes, and it's right, that's the style we usually use with multi-line macros.


================
Comment at: clang/include/clang/Sema/Sema.h:569
+  }
 
   // RAII object to push / pop sentinel slots for all MS #pragma stacks.
----------------
Spurious spaces (as pointed out by clang-format)


================
Comment at: clang/include/clang/Serialization/ASTWriter.h:509
   void WriteDeclContextVisibleUpdate(const DeclContext *DC);
-  void WriteFPPragmaOptions(const FPOptions &Opts);
+  void WriteFPPragmaOptions(const FPOptionsOverride &Opts);
   void WriteOpenCLExtensions(Sema &SemaRef);
----------------
I think it's telling you that you're missing a forward-declaration of this type in this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81869