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 method name? `diffFrom` would
> make it clear that the result, applied to the base, yields `this`.
I renamed this method to `getChanges`. I am not sure it becomes better. May be
`getChangesFrom` or `getDiffFrom`? Any suggestion is welcome.
================
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;
----------------
rjmccall wrote:
> 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.
It makes sense. Renamed to `InitialFPFeatures`.
================
Comment at: clang/lib/AST/StmtPrinter.cpp:216
+ LangOptions::RoundingMode RM = FPO.getConstRoundingModeOverride();
+ if (RM != llvm::RoundingMode::Dynamic) {
+ Indent() << "#pragma STDC FENV_ROUND ";
----------------
aaron.ballman wrote:
> Why is this not handled as well and print out `FE_DYNAMIC`?
Initially it was used to prevent extra `FENV_ROUND FE_DYNAMIC` pragma in the
case when only `FENV_ACCESS` was specified. However after landing D126364 there
is no need for that. Now `FE_DYNAMIC` is treated similar to other values.
================
Comment at: clang/lib/Basic/LangOptions.cpp:214
+ OverrideMask |= NAME##Mask;
+#include "clang/Basic/FPOptions.def"
+ return FPOptionsOverride(*this, OverrideMask);
----------------
rjmccall wrote:
> 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.
Implemented slow and inline versions. The later is defined after definition of
`FPOptionsOverride` in this file.
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
