[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

2 small things, @rjmccall  and @sepavloff , anything else?



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:870
+def err_pragma_fenv_requires_precise : Error<
+  "'#pragma STDC FENV_ACCESS ON' is illegal when precise is disabled">;
 def warn_cxx_ms_struct :
----------------
The last 4 can be done via selects as well!  Save a couple more spaces before we have to up the diagnostic id size :) 



================
Comment at: clang/include/clang/Sema/Sema.h:558
 
+  // This stacks the current state of Sema.CurFPFeatures
+  PragmaStack<unsigned> FpPragmaStack;
----------------
erichkeane wrote:
> This comment is really oddly phrased and uses the 'stack'-noun as a verb?  Something like: (please feel free to wordsmith): "This stack tracks the current state of Sema.CurFPFeatures."?
Just needs a period at the end.


================
Comment at: clang/lib/Parse/ParsePragma.cpp:2534
+  PP.Lex(Tok);
+  if (Tok.isNot(tok::l_paren)) {
+    PP.Diag(FloatControlLoc, diag::err_expected) << tok::l_paren;
----------------
mibintc wrote:
> erichkeane wrote:
> > Replace this with BalancedDelimiterTracker instead, it gives consistent errors and are a bit easier to use.  Additionally, I think it does some fixups that allow us to recover better.
> > 
> > I'd also suggest some refactoring with the PragmaFloatControlKind if/elses below.  Perhaps handle the closing paren at the end, and do a switch-case for that handling.
> BalancedDelimiterTracker doesn't work here because there's no access to the Parser object. Rewriting it would be an extensive change and I'm doubtful about making this change. PragmaHandler is defined in Lex. I think there are 60 pragma's that use the PragmaHandler. 
Thats unfortunate :/  That type does some nice fixup work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72841/new/

https://reviews.llvm.org/D72841