rsmith added inline comments.
================ Comment at: lib/AST/ExprConstant.cpp:4278-4287 + // Evaluate try blocks by evaluating all sub statements and keep track + // whether there's a return. + EvalStmtResult ESR = ESR_Succeeded; + for (const Stmt *SubStmt : S->children()) { + EvalStmtResult SubStmtESR = EvaluateStmt(Result, Info, SubStmt, Case); + if (SubStmtESR != ESR_Succeeded && SubStmtESR != ESR_Returned) + return ESR_Failed; ---------------- The children of a `try` statement are the `try` body followed by the `catch` statements, in order. We only want to evaluate the normal body, so you should just recurse to evaluating `cast<CXXTryStmt>(S)->getTryBlock()`... ================ Comment at: lib/AST/ExprConstant.cpp:4290-4294 + case Stmt::CXXCatchStmtClass: + // No need to evaluate catch since it will be ignored in case the try block + // is successfully evaluated. + return ESR_Succeeded; } ---------------- ... and this should then be unreachable. ================ Comment at: lib/Sema/SemaDeclCXX.cpp:1904-1905 + case Stmt::CXXTryStmtClass: + if (!SemaRef.getLangOpts().CPlusPlus2a) + break; + if (!Cxx1yLoc.isValid()) ---------------- Is there a reason to not allow this as an extension in earlier language modes? ================ Comment at: lib/Sema/SemaDeclCXX.cpp:1906-1907 + break; + if (!Cxx1yLoc.isValid()) + Cxx1yLoc = S->getBeginLoc(); + for (Stmt *SubStmt : S->children()) ---------------- `Cxx1yLoc` isn't right here; we should produce a -Wc++17-compat diagnostic, not a -Wc++14-compat diagnostic. We should probably prefer the `Cxx1zLoc` warning over the `Cxx1yLoc` warning if we have both. ================ Comment at: lib/Sema/SemaDeclCXX.cpp:1915-1922 + case Stmt::CXXCatchStmtClass: + if (!SemaRef.getLangOpts().CPlusPlus2a) + break; + if (!Cxx1yLoc.isValid()) + Cxx1yLoc = S->getBeginLoc(); + // In case we got a valid constexpr try block, the catch block can be + // ignored since it will never be evaluated in a constexpr context. ---------------- We should just unconditionally allow `CXXCatchStmt`s (that is, don't bother checking the language mode, just walk the children), since whenever we encounter one we must also have a `CXXTryStmt`, which we'll already have checked. ================ Comment at: lib/Sema/SemaDeclCXX.cpp:1962-1983 + // constexpr function try blocks are allowed in c++2a, assuming that the + // inner statements do also apply to general constexpr rules. + SourceLocation Cxx1yLoc; + for (Stmt *SubStmt : Body->children()) + if (SubStmt && + !CheckConstexprFunctionStmt(*this, Dcl, SubStmt, ReturnStmts, + Cxx1yLoc)) ---------------- The repetition can be avoided here by walking the children of `Body` (regardless of what kind of statement it is), and checking them all. (We can't *quite* just pass `Body` directly to `CheckConstexprFunctionStmt` because (nested) compound statements aren't permitted until C++14.) ================ Comment at: lib/Sema/SemaDeclCXX.cpp:1973 + << isa<CXXConstructorDecl>(Dcl); + return true; + } ---------------- Don't return here: we still need to do the other checks below. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55097/new/ https://reviews.llvm.org/D55097 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits