[PATCH] D123952: [FPEnv] Allow CompoundStmt to keep FP options

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Sema/ScopeInfo.h:77-78
 
-  CompoundScopeInfo(bool IsStmtExpr) : IsStmtExpr(IsStmtExpr) {}
+  /// FP options at the beginning of the compound statement, prior to
+  /// any pragma.
+  FPOptions FPFeatures;
----------------
sepavloff wrote:
> aaron.ballman wrote:
> > So these are the initial FPOptions inherited by the scope from its surrounding context? And it's never updated by a pragma?
> Yes, this FPOption is used to calculate the effect of pragmas in the compound statement as a difference with FPOptions stored in Sema, so that CompoundStmt keeps only FP features that are changed in it.
It might make sense to rename this to `OriginalFPFeatures` or `InitialFPFeatures` or something like that, to make it more clear that this field is not updated as we process the rest of the compound statement. WDYT?


================
Comment at: clang/lib/AST/Stmt.cpp:370-371
   setStmts(Stmts);
+  if (hasStoredFPFeatures())
+    setStoredFPFeatures(FPFeatures);
 }
----------------
There's a part of me that wonders if it's a slightly more clear design to have `setStoredFPFeatures()` set `CompoundStmtBits.HasFPFeatures` to true as needed rather than requiring the caller to do this two-step initialization process. WDYT?


================
Comment at: clang/lib/AST/StmtPrinter.cpp:216
+    LangOptions::RoundingMode RM = FPO.getConstRoundingModeOverride();
+    if (RM != llvm::RoundingMode::Dynamic) {
+      Indent() << "#pragma STDC FENV_ROUND ";
----------------
Why is this not handled as well and print out `FE_DYNAMIC`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123952