rsmith added a comment.
Thanks, this is definitely a step in the right direction.
================
Comment at: include/clang/AST/Expr.h:3257-3261
+ // Get the FENV_ACCESS status of this operator. Only meaningful for
+ // operations on floating point types.
+ bool isFENVAccessOn() const {
+ return FPOptions(FPFeatures).allowFENVAccess();
+ }
----------------
Nit: I think this should be capitalized as `FEnvAccess`. Lowercasing the
"ccess" of "access" but not the "nv" of "environment" seems inconsistent to me,
and falsely makes "FENV" look like an initialism or acronym.
================
Comment at: include/clang/Basic/LangOptions.h:273-280
FPOptions() : fp_contract(LangOptions::FPC_Off) {}
// Used for serializing.
explicit FPOptions(unsigned I)
: fp_contract(static_cast<LangOptions::FPContractModeKind>(I)) {}
explicit FPOptions(const LangOptions &LangOpts)
----------------
These constructors need to be updated.
================
Comment at: lib/Parse/ParsePragma.cpp:619-623
+#if NOTYET // FIXME: Add this cli option when it makes sense.
+ case tok::OOS_DEFAULT:
+ FPC = getLangOpts().getDefaultFENVAccessMode();
+ break;
+#endif
----------------
You need to do *something* in this case. Currently, `FPC` is read uninitialized
a few lines below when this happens. How about just treating this as the same
as `OFF` for now, since that is our default.
================
Comment at: lib/Parse/ParseStmt.cpp:353
+ ProhibitAttributes(Attrs);
+ //Diag(Tok, diag::err_pragma_fp_scope);
+ HandlePragmaFENVAccess();
----------------
Delete this line.
================
Comment at: lib/Sema/SemaAttr.cpp:779
+ case LangOptions::FEA_On:
+ FPFeatures.setAllowFENVAccess();
+ break;
----------------
Not directly related to your patch, but... treating `FPFeatures` as persistent
`Sema` state will be error-prone. For example, we'll get the wrong features in
template instantiation and implicitly-generated special member functions. But I
don't have a good alternative to suggest right now (other than tracking down
the places where we need to save and restore this extra state and doing so), so
this is just a concern, not a call to action at this point.
Repository:
rC Clang
https://reviews.llvm.org/D49865
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
