Fix getLocEnd for function declarations with exception specification.

[adek05]

Summary

Instead of pointing at the current token, which is a semicolon, read the last token from the stack of parsed tokens.

[adek05]

Tests are in D15444. I do not have a good test plan for this in clang source tree. If you have an idea, let me know.

[adek05]

Ping

[aaron.ballman]

Sorry for the delay in reviewing this!

In terms of a test case, presumably you need this because a diagnostic location is incorrect -- would it be possible to write the exception specification such that it is on its own line, and then expect-warning|error|note on the new line instead? e.g.,

void f()
noexcept(that does something diagnosable); // expected-warning {{some diagnostic}}

I'm not too keen on adding the change without a test case because we may accidentally break it again in the future. However, the general gist of the code looks correct to me.

[adek05]

I think this won't work. If there is a problem with the expression inside throw or noexcept specifier, it will be highlighted inside the parens, never trying to actually access the end location of the function declaration.

It would be enough if there is a warning/error which would highlight the whole function declaration, because we could check whether it points at ; or the character in front of it, but I don't know about such warning and I don't know how to smartly look for it.

[aaron.ballman]

In D15443#323043, @adek05 wrote:

I think this won't work. If there is a problem with the expression inside throw or noexcept specifier, it will be highlighted inside the parens, never trying to actually access the end location of the function declaration.

It would be enough if there is a warning/error which would highlight the whole function declaration, because we could check whether it points at ; or the character in front of it, but I don't know about such warning and I don't know how to smartly look for it.

I think the best way to proceed then is with a test in SourceLocationTest.cpp in the AST unit test suite.

[adek05]

Adding testcases in unittest/AST/SourceLocationTest.cpp as suggested by @aaronballman

Interestingly, without my change tests for function declarations pass. Only member functions fail:

tools/clang/unittests/AST/ASTTests
...
[----------] 2 tests from FunctionDecl
[ RUN      ] FunctionDecl.FunctionDeclWithThrowSpecification
[       OK ] FunctionDecl.FunctionDeclWithThrowSpecification (17 ms)
[ RUN      ] FunctionDecl.FunctionDeclWithNoExceptSpecification
[       OK ] FunctionDecl.FunctionDeclWithNoExceptSpecification (10 ms)
[----------] 2 tests from FunctionDecl (27 ms total)

[----------] 2 tests from CXXMethodDecl
[ RUN      ] CXXMethodDecl.CXXMethodDeclWithThrowSpecification
/Users/adek/llvm-git/tools/clang/unittests/AST/SourceLocationTest.cpp:569: Failure
Value of: Verifier.match( "class A {\n" "void f() throw();\n" "};\n", functionDecl())
  Actual: false (Expected range <2:1-2:16>, found <input.cc:2:1-input.cc:2:17>)
Expected: true
[  FAILED  ] CXXMethodDecl.CXXMethodDeclWithThrowSpecification (10 ms)
[ RUN      ] CXXMethodDecl.CXXMethodDeclWithNoExceptSpecification
/Users/adek/llvm-git/tools/clang/unittests/AST/SourceLocationTest.cpp:580: Failure
Value of: Verifier.match( "class A {\n" "void f() noexcept(false);\n" "};\n", functionDecl(), Language::Lang_CXX11)
  Actual: false (Expected range <2:1-2:24>, found <input.cc:2:1-input.cc:2:25>)
Expected: true
[  FAILED  ] CXXMethodDecl.CXXMethodDeclWithNoExceptSpecification (10 ms)
[----------] 2 tests from CXXMethodDecl (20 ms total)

Not sure why would they take different codepaths, throw and noexcept are C++(11) specific so both should go through ParseDeclCXX.

[aaron.ballman]

In D15443#324426, @adek05 wrote:

Adding testcases in unittest/AST/SourceLocationTest.cpp as suggested by @aaronballman

Interestingly, without my change tests for function declarations pass. Only member functions fail:

tools/clang/unittests/AST/ASTTests
...
[----------] 2 tests from FunctionDecl
[ RUN      ] FunctionDecl.FunctionDeclWithThrowSpecification
[       OK ] FunctionDecl.FunctionDeclWithThrowSpecification (17 ms)
[ RUN      ] FunctionDecl.FunctionDeclWithNoExceptSpecification
[       OK ] FunctionDecl.FunctionDeclWithNoExceptSpecification (10 ms)
[----------] 2 tests from FunctionDecl (27 ms total)

[----------] 2 tests from CXXMethodDecl
[ RUN      ] CXXMethodDecl.CXXMethodDeclWithThrowSpecification
/Users/adek/llvm-git/tools/clang/unittests/AST/SourceLocationTest.cpp:569: Failure
Value of: Verifier.match( "class A {\n" "void f() throw();\n" "};\n", functionDecl())
  Actual: false (Expected range <2:1-2:16>, found <input.cc:2:1-input.cc:2:17>)
Expected: true
[  FAILED  ] CXXMethodDecl.CXXMethodDeclWithThrowSpecification (10 ms)
[ RUN      ] CXXMethodDecl.CXXMethodDeclWithNoExceptSpecification
/Users/adek/llvm-git/tools/clang/unittests/AST/SourceLocationTest.cpp:580: Failure
Value of: Verifier.match( "class A {\n" "void f() noexcept(false);\n" "};\n", functionDecl(), Language::Lang_CXX11)
  Actual: false (Expected range <2:1-2:24>, found <input.cc:2:1-input.cc:2:25>)
Expected: true
[  FAILED  ] CXXMethodDecl.CXXMethodDeclWithNoExceptSpecification (10 ms)
[----------] 2 tests from CXXMethodDecl (20 ms total)

Not sure why would they take different codepaths, throw and noexcept are C++(11) specific so both should go through ParseDeclCXX.

It would be nice to understand why that is the case and whether there's some code sharing possibilities, but since this is such a small change, I'm not certain of the gains.

LGTM! Thanks!

[adek05]

Would you mind committing it for me, and I don't have a commit access yet.
Also, could you take a look at D15444 with tests for clang-tidy related to this change?

[aaron.ballman]

Commit in r257521

lib/Parse/ParseDeclCXX.cpp

if (Delayed) {
     ExceptionSpecTokens = new CachedTokens;
     ExceptionSpecTokens->push_back(StartTok); // 'throw' or 'noexcept'
     ExceptionSpecTokens->push_back(Tok); // '('
     SpecificationRange.setEnd(ConsumeParen()); // '('
 
     ConsumeAndStoreUntil(tok::r_paren, *ExceptionSpecTokens,
                          /*StopAtSemi=*/true,
                          /*ConsumeFinalToken=*/true);
-    SpecificationRange.setEnd(Tok.getLocation());
+    SpecificationRange.setEnd(ExceptionSpecTokens->back().getLocation());
+
     return EST_Unparsed;
   }
   
   // See if there's a dynamic specification.
   if (Tok.is(tok::kw_throw)) {
     Result = ParseDynamicExceptionSpecification(SpecificationRange,
                                                 DynamicExceptions,
                                                 DynamicExceptionRanges);

unittests/AST/SourceLocationTest.cpp

EXPECT_TRUE(Verifier.match(
       "struct A { int a; };\n"
       "@interface B {}\n"
       "+ (void) f1: (A)arg;\n"
       "@end\n"
       "void f2() { A a; [B f1: (a)]; }\n",
       cxxConstructExpr(), Lang_OBJCXX));
 }
 
+TEST(FunctionDecl, FunctionDeclWithThrowSpecification) {
+  RangeVerifier<FunctionDecl> Verifier;
+  Verifier.expectRange(1, 1, 1, 16);
+  EXPECT_TRUE(Verifier.match(
+      "void f() throw();\n",
+      functionDecl()));
+}
+
+TEST(FunctionDecl, FunctionDeclWithNoExceptSpecification) {
+  RangeVerifier<FunctionDecl> Verifier;
+  Verifier.expectRange(1, 1, 1, 24);
+  EXPECT_TRUE(Verifier.match(
+      "void f() noexcept(false);\n",
+      functionDecl(),
+      Language::Lang_CXX11));
+}
+
+TEST(CXXMethodDecl, CXXMethodDeclWithThrowSpecification) {
+  RangeVerifier<FunctionDecl> Verifier;
+  Verifier.expectRange(2, 1, 2, 16);
+  EXPECT_TRUE(Verifier.match(
+      "class A {\n"
+      "void f() throw();\n"
+      "};\n",
+      functionDecl()));
+}
+
+TEST(CXXMethodDecl, CXXMethodDeclWithNoExceptSpecification) {
+  RangeVerifier<FunctionDecl> Verifier;
+  Verifier.expectRange(2, 1, 2, 24);
+  EXPECT_TRUE(Verifier.match(
+      "class A {\n"
+      "void f() noexcept(false);\n"
+      "};\n",
+      functionDecl(),
+      Language::Lang_CXX11));
+}
+
 } // end namespace ast_matchers
 } // end namespace clang