[Sema] Handle invalid noexcept expressions correctly.

[hintonda]

Summary

Treat invalid noexcept specifications in the same way we
treat invalid throw specifications, by not resetting the exception
type to EST_None, and testing isUsable instead of isInvalid.

Rational: In order to add source locations for exception
specifications (see https://reviews.llvm.org/D20428) we need to
maintain the correct exception type, otherwise, FunctionDecl types
won't include space for source ranges, but the associated
TypeSourceInfo type will, causing ASTContext::adjustExceptionSpec to
assert once source ranges have been added.

[rsmith]

lib/Parse/ParseDeclCXX.cpp

3547 ↗

(On Diff #82959)

Should NoexceptRange be set in the else case too, now that we're claiming that the type is EST_ComputedNoexcept?

lib/Sema/TreeTransform.h

5044–5057 ↗

(On Diff #82959)

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?

[hintonda]

lib/Parse/ParseDeclCXX.cpp

3547 ↗

(On Diff #82959)

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.

lib/Sema/TreeTransform.h

5044–5057 ↗

(On Diff #82959)

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.

[hintonda]

Richard, what do you think about just avoiding the problem altogether by only calling actOnDelayedExceptionSpecification is noexcept type != EST_None?

[hintonda]

Rollback previous change and instead only call
actOnDelayedExceptionSpecification if noexcept type is not EST_None.

[rsmith]

Looks OK. Is it possible to add a test case for this without D20428? If not, this is small enough that rolling it into D20428 (so it can be committed with its testcase) would make sense.

[hintonda]

Unable to test change here, so have included fix directly in D20428.

lib/Parse/ParseCXXInlineMethods.cpp

ExceptionSpecificationType EST
       = tryParseExceptionSpecification(/*Delayed=*/false, SpecificationRange,
                                        DynamicExceptions,
                                        DynamicExceptionRanges, NoexceptExpr,
                                        ExceptionSpecTokens);
 
     if (Tok.isNot(tok::eof) || Tok.getEofData() != LM.Method)
       Diag(Tok.getLocation(), diag::err_except_spec_unparsed);
 
+    if (EST != EST_None) {
       // Attach the exception-specification to the method.
       Actions.actOnDelayedExceptionSpecification(LM.Method, EST,
                                                  SpecificationRange,
                                                  DynamicExceptions,
                                                  DynamicExceptionRanges,
                                                  NoexceptExpr.isUsable()?
                                                    NoexceptExpr.get() : nullptr);
+    }
 
     // There could be leftover tokens (e.g. because of an error).
     // Skip through until we reach the original token position.
     while (Tok.isNot(tok::eof))
       ConsumeAnyToken();
 
     // Clean up the remaining EOF token.
     if (Tok.is(tok::eof) && Tok.getEofData() == LM.Method)