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

[Bruno Ricci via Phabricator via llvm-commits]

riccibruno added inline comments.


================
Comment at: clang/include/clang/AST/Stmt.h:620
+    //unsigned FPFeatures : 21;
+    unsigned FPFeatures : 32;
   };
----------------
mibintc wrote:
> This is a temporary change, I know you don't want me to change the assert to allow a wider size than 8. So if this information is really needed (as I mentioned above it's not being used in Sema or Codegen) then alternatives are
> 1. rewrite the == comparison using BinaryOperator
> 2. put the FPFeatures Override into TrailingStorage (i think i will need implementation guidance if this is the path to go)
What do you mean by `rewrite the == comparison using BinaryOperator`? If you mean representing overloaded `operator==`'s by `BinaryOperator`, then that's certainly not right because `BinaryOperator`s only represent binary builtin operators.

You will not be able to use `TrailingObjects` with `CXXOperatorCallExpr` because `CallExpr` stores its arguments after the object containing the `CallExpr` sub-object. You could manually stash it after `CallExpr`'s arguments, but that would be pretty ugly.

Do you really need 32-bits worth of FP options? I see from `FPOptions.def` that you need 14 bits, so it should fit with room to spare.


================
Comment at: clang/include/clang/AST/Stmt.h:1121
   Stmt(StmtClass SC) {
-    static_assert(sizeof(*this) <= 8,
+    static_assert(sizeof(*this) <= 16,
                   "changing bitfields changed sizeof(Stmt)");
----------------
mibintc wrote:
> this is a temporary change
It better be :)


================
Comment at: clang/include/clang/Basic/LangOptions.h:455
+  llvm::errs() << "\n RoundingMode " <<
+        static_cast<unsigned>(getRoundingMode());
+  llvm::errs() << "\n FPExceptionMode " << getFPExceptionMode();
----------------
mibintc wrote:
> Using #define OPTION trick would be preferrable except there is a compilation failure because something funny about RoundingMode--need the cast. doubtless there is fix for that
Yes it would be preferable and it should not be in a header too.

You need the cast because `RoundingMode` is a scoped enumeration, which does not have the implicit conversions of unscoped enumerations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81869