[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    Re: [PATCH] D11789: Modify DeclaratorChuck::getFunction to be passed an Exception Specification Sour
From:       Richard Smith <richard () metafoo ! co ! uk>
Date:       2015-08-06 0:45:46
Message-ID: d1b0424968841eabedf54d46cfdeb4da () localhost ! localdomain
[Download RAW message or body]

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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]