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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
