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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb822efc7404b: [FPEnv] Allow CompoundStmt to keep FP options (authored by sepavloff). Changed prior to commit: https://reviews.llvm.org/D123952?vs=

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

sepavloff added a comment. Herald added a reviewer: NoQ. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123952/new/ https://reviews.llvm.org/D123952 ___ cfe-commits mailing list cfe-commits@lists.

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

rjmccall added inline comments. Comment at: clang/lib/AST/Stmt.cpp:370-371 setStmts(Stmts); + if (hasStoredFPFeatures()) +setStoredFPFeatures(FPFeatures); } aaron.ballman wrote: > rjmccall wrote: > > aaron.ballman wrote: > > > There's a part of me that w

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

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM modulo the rename request. Comment at: clang/lib/AST/Stmt.cpp:370-371 setStmts(Stmts); + if (hasStoredFPFeatures()) +setStoredFPFeatures(FPFeatures

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

rjmccall added a comment. Looks good, thanks! Approved with the rename as discussed. Comment at: clang/include/clang/Basic/LangOptions.h:747 + /// Return difference with the given option set. + FPOptionsOverride diffWith(const FPOptions &Base); + sepavloff w

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

sepavloff added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:747 + /// Return difference with the given option set. + FPOptionsOverride diffWith(const FPOptions &Base); + rjmccall wrote: > Can you make direction more obvious in the metho

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

sepavloff updated this revision to Diff 440644. sepavloff added a comment. Updated patch according to reviewers' notes - Rename field of CompoundScopeInfo, - Treat FE_DYNAMIC like other rounding mode arguments in StmtPrinter, - Rename and optimize method for calculation of FP option difference.

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

rjmccall added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:747 + /// Return difference with the given option set. + FPOptionsOverride diffWith(const FPOptions &Base); + Can you make direction more obvious in the method name? `diffFrom`

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

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; -

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

sepavloff added inline comments. Comment at: clang/lib/Basic/LangOptions.cpp:214 +OverrideMask |= NAME##Mask; +#include "clang/Basic/FPOptions.def" + return FPOptionsOverride(*this, OverrideMask); rjmccall wrote: > Hmm. If we can assume that all of the opti

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

rjmccall added inline comments. Comment at: clang/lib/Basic/LangOptions.cpp:214 +OverrideMask |= NAME##Mask; +#include "clang/Basic/FPOptions.def" + return FPOptionsOverride(*this, OverrideMask); Hmm. If we can assume that all of the options are single bits

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

sepavloff added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:452 + : getCurCompoundScope().FPFeatures; + FPOptionsOverride FPDiff = getCurFPFeatures().diffWith(FPO); + rjmccall wrote: > How cheap is this? Is this something it wo

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

sepavloff updated this revision to Diff 440241. sepavloff added a comment. Rebased, optimized FPOptions::diffWith. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123952/new/ https://reviews.llvm.org/D123952 Files: clang/include/clang/AST/JSONNode

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

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:452 + : getCurCompoundScope().FPFeatures; + FPOptionsOverride FPDiff = getCurFPFeatures().diffWith(FPO); + How cheap is this? Is this something it would be worthwhile t

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

sepavloff updated this revision to Diff 431294. sepavloff added a comment. Update after D125635 and rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123952/new/ https://reviews.llvm.org/D123952 Files: clang

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

sepavloff added a comment. In D123952#3488638 , @aaron.ballman wrote: > 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 othe

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

sepavloff updated this revision to Diff 429525. sepavloff added a comment. Addressed review notes - Added support in JSON node dumper, - Added support in ast-print Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123952/new/ https://reviews.llvm.org/

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

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 di

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

sepavloff created this revision. sepavloff added reviewers: rsmith, rjmccall, aaron.ballman, kpn. Herald added subscribers: dexonsmith, martong. Herald added a reviewer: shafik. Herald added a project: All. sepavloff requested review of this revision. Herald added a project: clang. AST does not ha