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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits