Page MenuHomePhabricator

Fix getLocEnd for function declarations with exception specification.
ClosedPublic

Authored by adek05 on Dec 11 2015, 12:15 AM.

Details

Summary

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

Diff Detail

Event Timeline

adek05 updated this revision to Diff 42504.Dec 11 2015, 12:15 AM
adek05 retitled this revision from to Fix getLocEnd for function declarations with exception specification..
adek05 updated this object.
adek05 added a reviewer: rsmith.

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 removed a reviewer: cfe-commits.
adek05 added a subscriber: cfe-commits.
aaron.ballman edited edge metadata.Jan 6 2016, 2:45 PM

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 added a comment.Jan 9 2016, 9:57 PM

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 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 updated this revision to Diff 44598.EditedJan 11 2016, 8:53 PM
adek05 edited edge metadata.

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 accepted this revision.Jan 12 2016, 5:35 AM
aaron.ballman edited edge metadata.

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!

This revision is now accepted and ready to land.Jan 12 2016, 5:35 AM

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 closed this revision.Jan 12 2016, 1:08 PM

Commit in r257521