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

[Melanie Blower via Phabricator via llvm-commits]

mibintc marked an inline comment as done.
mibintc added inline comments.


================
Comment at: clang/include/clang/AST/Expr.h:2701
 
+  FPOptions FPFeatures;
+
----------------
mibintc wrote:
> erichkeane wrote:
> > This type already has trailing-storage type stuff.  I think in the past review @rjmccall encouraged you to put this in the fake-trailing-storage  like above.
> whoops i meant that to go to the CallExprBits
Adding FPOptions to CallExprBits makes the field too large, I'm going to drop adding FPOptions to CallExpr, i'll add it via trailingstorage in a separate patch if it's needed.


================
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;
----------------
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. 


================
Comment at: clang/lib/Sema/SemaAttr.cpp:1023
+      Diag(Loc, diag::err_pragma_fenv_requires_precise);
     CurFPFeatures.setAllowFEnvAccess();
     break;
----------------
erichkeane wrote:
> Should we still be setting this even if there was an error?
> Should we still be setting this even if there was an error?
It's not harmful to set it, if there's an error diagnostic then there is no codegen right?




================
Comment at: clang/lib/Serialization/ASTReaderStmt.cpp:684
 void ASTStmtReader::VisitUnaryOperator(UnaryOperator *E) {
+  bool hasFP_Features;
   VisitExpr(E);
----------------
mibintc wrote:
> erichkeane wrote:
> > Rather than this variable, why not just ask 'E' below?
> yes i could do that. it would be a function call
i made some changes in this area, eliminating setHasStoredFPFeatures


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72841