[PATCH] D28258: [Sema] Handle invalid noexcept expressions correctly.

[don hinton via Phabricator via cfe-commits]

hintonda added inline comments.


================
Comment at: lib/Parse/ParseDeclCXX.cpp:3547
       NoexceptRange = SourceRange(KeywordLoc, T.getCloseLocation());
-    } else {
-      NoexceptType = EST_None;
     }
   } else {
----------------
rsmith wrote:
> Should `NoexceptRange` be set in the `else` case too, now that we're claiming that the type is `EST_ComputedNoexcept`?
If the expression is invalid, it's never used.  In fact, since it is invalid, it may not be possible to compute the range.  However, I'll check that.


================
Comment at: lib/Sema/TreeTransform.h:5044-5057
+    if (!NoexceptExpr.isUsable())
       return true;
 
     // FIXME: This is bogus, a noexcept expression is not a condition.
     NoexceptExpr = getSema().CheckBooleanCondition(Loc, NoexceptExpr.get());
-    if (NoexceptExpr.isInvalid())
+    if (!NoexceptExpr.isUsable())
       return true;
----------------
rsmith wrote:
> These changes don't make sense to me: if we get a valid-but-null `ExprResult` from any of the above, there is no guarantee a diagnostic has been produced, so it is not correct to return `true`.
> 
> Which call is producing the valid-but-null `ExprResult`?
If the expression contains an undefined value, then ParseConstantExpression, and the functions it calls, will produce diagnostics and return an invalid ExprResult, i.e., 0x01.  That means you can't call CheckBooleanCondition because it assumes the pointer is good.  The problem is that this information is lost once tryParseExceptionSpecification returns.  From then on, it's stored as a null value, i.e., as if it never existed which corresponds to EST_None.

However, we need to know that it did in fact exist so we can get the sizes of FunctionDecl type and TypeSourceInfo type to match, and that won't happen if we set the exception type to EST_None.

So, if we keep EST_ComputedNoexcept, we need to actually test the pointer before we use it -- isValid is no longer good enough.



https://reviews.llvm.org/D28258