[PATCH] D49865: Inform the AST of #pragma FENV_ACCESS use

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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