[PATCH] D11789: Modify DeclaratorChuck::getFunction to be passed an Exception Specification SourceRange

[Richard Smith]

rsmith added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1977
@@ -1976,1 +1976,3 @@
+def  err_function_concept_exception_spec : Error<
+  "function concept can not have exception specifiers">;
 
----------------
def  err -> def err
can not -> cannot
specifiers -> specification

================
Comment at: include/clang/Sema/DeclSpec.h:1330-1336
@@ -1325,5 +1329,9 @@
 
-    SourceLocation getExceptionSpecLoc() const {
-      return SourceLocation::getFromRawEncoding(ExceptionSpecLoc);
+    SourceLocation getExceptionSpecLocBeg() const {
+      return SourceLocation::getFromRawEncoding(ExceptionSpecLocBeg);
+    }
+
+    SourceLocation getExceptionSpecLocEnd() const {
+      return SourceLocation::getFromRawEncoding(ExceptionSpecLocEnd);
     }
 
----------------
Please add a `getExceptionSpecRange` function returning the `SourceRange`...

================
Comment at: lib/Sema/SemaDecl.cpp:7447-7450
@@ -7446,1 +7446,6 @@
 
+      if (const FunctionProtoType *FPT = R->getAs<FunctionProtoType>()) {
+        if (FPT->hasExceptionSpec()) {
+          auto LocBeg = D.getFunctionTypeInfo().getExceptionSpecLocBeg();
+          auto LocEnd = D.getFunctionTypeInfo().getExceptionSpecLocEnd();
+          Diag(LocBeg, diag::err_function_concept_exception_spec)
----------------
This will assert if there isn't a `FunctionTypeInfo` for the declaration, which can theoretically happen if it's declared via an (ill-formed today) `typedef`. (It also might not provide a source range if the exception specification is implicit, for instance because the function template is a destructor or deallocation function, but passing an empty SourceRange to the FixItHint should just result it in being ignored.)

================
Comment at: lib/Sema/SemaDecl.cpp:7449-7452
@@ +7448,6 @@
+        if (FPT->hasExceptionSpec()) {
+          auto LocBeg = D.getFunctionTypeInfo().getExceptionSpecLocBeg();
+          auto LocEnd = D.getFunctionTypeInfo().getExceptionSpecLocEnd();
+          Diag(LocBeg, diag::err_function_concept_exception_spec)
+            << FixItHint::CreateRemoval(SourceRange(LocBeg, LocEnd));
+          NewFD->setInvalidDecl();
----------------
... and use it here.

================
Comment at: lib/Sema/SemaDecl.cpp:7454-7455
@@ +7453,4 @@
+          NewFD->setInvalidDecl();
+        }
+        else {
+          NewFD->setType(Context.getFunctionType(
----------------
No newline before `else`.

================
Comment at: lib/Sema/SemaDecl.cpp:7456
@@ +7455,3 @@
+        else {
+          NewFD->setType(Context.getFunctionType(
+            FPT->getReturnType(), FPT->getParamTypes(),
----------------
rsmith wrote:
> Use `ASTContext::adjustExceptionSpec(NewFD, EST_BasicNoexcept)` for this.
Add a comment here indicating why we're doing this. Maybe a snippet of the relevant rule from the TS?

================
Comment at: lib/Sema/SemaDecl.cpp:7456-7458
@@ +7455,5 @@
+        else {
+          NewFD->setType(Context.getFunctionType(
+            FPT->getReturnType(), FPT->getParamTypes(),
+            FPT->getExtProtoInfo().withExceptionSpec(EST_BasicNoexcept)));
+        }
----------------
Use `ASTContext::adjustExceptionSpec(NewFD, EST_BasicNoexcept)` for this.

================
Comment at: lib/Sema/SemaDecl.cpp:7456-7458
@@ +7455,5 @@
+        else {
+          NewFD->setType(Context.getFunctionType(
+            FPT->getReturnType(), FPT->getParamTypes(),
+            FPT->getExtProtoInfo().withExceptionSpec(EST_BasicNoexcept)));
+        }
----------------
rsmith wrote:
> rsmith wrote:
> > Use `ASTContext::adjustExceptionSpec(NewFD, EST_BasicNoexcept)` for this.
> Add a comment here indicating why we're doing this. Maybe a snippet of the relevant rule from the TS?
You don't seem to have test coverage for this part. Maybe test it with `static_assert(noexcept(SomeConcept<T>()));`?


http://reviews.llvm.org/D11789