Page MenuHomePhabricator

ASTMatchers: enable hasBody() matcher for FunctionDecls
ClosedPublic

Authored by a.sidorin on Jan 15 2016, 2:40 AM.

Details

Summary

This patch enables matching for FunctionDecls that have bodies.

Diff Detail

Event Timeline

a.sidorin updated this revision to Diff 44969.Jan 15 2016, 2:40 AM
a.sidorin retitled this revision from to ASTMatchers: enable hasBody() matcher for FunctionDecls.
a.sidorin updated this object.
a.sidorin added reviewers: klimek, aaron.ballman.
a.sidorin added a subscriber: cfe-commits.
a.sidorin updated this revision to Diff 44975.Jan 15 2016, 3:19 AM

Update comments and documentation.

aaron.ballman edited edge metadata.Jan 15 2016, 6:16 AM

The examples and test code you use seem to imply that the isDefinition() matcher may be all you need; is isDefinition() insufficient for some reason? If so, can you expand the test cases to cover that particular usage? Also, FunctionDecl::getBody() does more work than FunctionDecl::hasBody() -- it would be better to implement the AST matcher in terms of FunctionDecl::hasBody() instead.

a.sidorin updated this revision to Diff 45232.Jan 19 2016, 1:31 AM
a.sidorin edited edge metadata.

Match only FunctionDecls which are definitions and ignore redeclarations without bodies.

aaron.ballman added inline comments.Jan 19 2016, 7:47 AM
include/clang/ASTMatchers/ASTMatchers.h
3118

s/declaration/definition.

include/clang/ASTMatchers/ASTMatchersInternal.h
1590

Indentation here is incorrect, you should run clang-format over the patch.

lib/ASTMatchers/ASTMatchersInternal.cpp
342 ↗(On Diff #45232)

The specialization should live in the header file, not the source file.

344 ↗(On Diff #45232)

I would use Node.doesThisDeclarationHaveABody() instead; otherwise this will match definitions that are explicitly deleted, which isn't particularly useful.

a.sidorin updated this revision to Diff 45267.Jan 19 2016, 8:29 AM

Fix issues pointed on review.

aaron.ballman accepted this revision.Jan 19 2016, 10:00 AM
aaron.ballman edited edge metadata.

With one small nit, LGTM, thanks!

include/clang/ASTMatchers/ASTMatchersInternal.h
1596

s/NULL/nullptr

This revision is now accepted and ready to land.Jan 19 2016, 10:00 AM
a.sidorin updated this revision to Diff 45360.Jan 20 2016, 12:47 AM
a.sidorin edited edge metadata.

Replace NULL with nullptr.

aaron.ballman closed this revision.Jan 20 2016, 8:30 AM

Thanks! I went ahead and commit in r258322.