[PATCH] D55097: [constexpr][c++2a] Try-catch blocks in constexpr functions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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