This is an archive of the discontinued LLVM Phabricator instance.

Add hasTrailingReturn AST matcher
ClosedPublic

Authored by juliehockett on Jan 18 2018, 4:52 PM.

Details

Diff Detail

Event Timeline

juliehockett created this revision.Jan 18 2018, 4:52 PM
aaron.ballman added inline comments.Jan 19 2018, 5:52 AM
include/clang/ASTMatchers/ASTMatchers.h
5902

I think this may cause failed assertions on code like void f(); when compiled in C mode because that FunctionDecl should have a type of FunctionNoProtoType. You should use getAs<FunctionProtoType>() and test for null.

unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
2121

Spurious semicolon in the test.

juliehockett marked 2 inline comments as done.

Updating matcher and fixing semicolons

aaron.ballman accepted this revision.Jan 19 2018, 11:56 AM

LGTM with a minor commenting/documentation nit.

include/clang/ASTMatchers/ASTMatchers.h
5898

Sorry for not noticing this earlier -- spurious semicolon here (don't forget to regenerate the docs too).

This revision is now accepted and ready to land.Jan 19 2018, 11:56 AM
lebedev.ri added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
5904

There are no negative tests in the unittest that cover this false path.

juliehockett added inline comments.Jan 19 2018, 4:26 PM
include/clang/ASTMatchers/ASTMatchers.h
5904

Is there a test case you would recommend? I'm not entirely sure what would be appropriate -- the tests compile in C++, yes? So void f(); would just be a normal function declaration (with a prototype, please correct me if I'm wrong).

lebedev.ri added inline comments.Jan 20 2018, 12:16 AM
include/clang/ASTMatchers/ASTMatchers.h
5904

I'd start by checking what @aaron.ballman has suggested:

EXPECT_TRUE(notMatches("void f();"));
aaron.ballman added inline comments.Jan 20 2018, 4:54 AM
include/clang/ASTMatchers/ASTMatchers.h
5904

You'll need to use notMatchesC() to test that case (so you get a C input file instead of a C++ input file).

lebedev.ri added inline comments.Jan 20 2018, 6:53 AM
include/clang/ASTMatchers/ASTMatchers.h
5904

Oh, sorry, indeed.
I did mean notMatchesC(), the trailing C was lost during copy-paste.

Adding test case -- thank you for the advice!

This revision was automatically updated to reflect the committed changes.