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` would make
it clear that the result, applied to the base, yields `this`.
================
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;
----------------
aaron.ballman wrote:
> 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?
Yeah, I agree; this name makes it sound like the current, live set of features,
even if the comment explains that it isn't.
================
Comment at: clang/lib/AST/Stmt.cpp:370-371
setStmts(Stmts);
+ if (hasStoredFPFeatures())
+ setStoredFPFeatures(FPFeatures);
}
----------------
aaron.ballman wrote:
> 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?
`setStoredFPFeatures` is only otherwise used in deserialization, and the node
has to be allocated properly to support it. I think this is the right approach.
================
Comment at: clang/lib/Basic/LangOptions.cpp:214
+ OverrideMask |= NAME##Mask;
+#include "clang/Basic/FPOptions.def"
+ return FPOptionsOverride(*this, OverrideMask);
----------------
sepavloff wrote:
> rjmccall wrote:
> > Hmm. If we can assume that all of the options are single bits (which this
> > is doing), then this algorithm is basically `FPOptions::storage_type
> > OverrideMask = (Value ^ Base.Value);`, which makes this cheap enough to be
> > defined inline in the header.
> `FPContractMode`, `ConstRoundingMode`, `SpecifiedExceptionMode` and
> `FPEvalMethod` occupy more than one bit:
> https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/FPOptions.def.
Oh, I see. I wasn't reasoning correctly about how this is a mask and not a
value.
In non-LTO builds, we'll have to actually do this call when leaving every
scope. It's a minor overhead, but it'd be nice to avoid. Please do the fast
path in an inline implementation and then put this slow path out of line, in a
private method named `diffFromSlow` or something like that.
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.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
