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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thanks for working on this! One thing that's not clear to me is what bugs this is solving -- the test coverage only shows a change to textual AST dumping behavior. Is this otherwise expected to be an NFC change, or should there be some codegen tests that show a difference in behavior?



================
Comment at: clang/include/clang/AST/Stmt.h:134
+    /// floating-point features.
+    unsigned HasFPFeatures : 1;
+
----------------
I don't think this is a bad approach, but it does further reduce the number of statements we support in a compound statement.

There's a part of me that wonders if the approach is to introduce a new AST node for a stateful compound statement; I suspect if we dig around, we'll find other pragmas that want to behave similar to floating-point feature pragmas (the idea of a pragma scoped to a block is not really new). Having something more general means we're less likely to keep stealing bits here.


================
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;
----------------
So these are the initial FPOptions inherited by the scope from its surrounding context? And it's never updated by a pragma?


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:2372-2376
+void TextNodeDumper::VisitCompoundStmt(const CompoundStmt *S) {
+  VisitStmt(S);
+  if (S->hasStoredFPFeatures())
+    printFPOptions(S->getStoredFPFeatures());
+}
----------------
Should we have a similar change for the JSON node dumper?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123952