This is an archive of the discontinued LLVM Phabricator instance.

ASTMatchers: add new statement/decl matchers
ClosedPublic

Authored by a.sidorin on Feb 19 2016, 6:33 AM.

Details

Reviewers
aaron.ballman
Summary

Added matchers:
addrLabelExpr
atomicExpr
binaryConditionalOperator
designatedInitExpr
designatorCountIs
hasSyntacticForm
implicitValueInitExpr
labelDecl
opaqueValueExpr
parenListExpr
predefinedExpr
requiresZeroInitialization
stmtExpr

Enable ConditionOperator-related matchers for BinaryConditionalOperator also
Enable hasSourceExpression() for OpaqueValueExpr

Diff Detail

Event Timeline

a.sidorin updated this revision to Diff 48482.Feb 19 2016, 6:33 AM
a.sidorin retitled this revision from to ASTMatchers: add new statement/decl matchers.
a.sidorin updated this object.
a.sidorin added a reviewer: aaron.ballman.
a.sidorin set the repository for this revision to rL LLVM.
a.sidorin added a subscriber: cfe-commits.
aaron.ballman edited edge metadata.Feb 19 2016, 6:58 AM

Given the wide disparity of functionality, I'm wondering if there are concrete purposes for these new matchers?

include/clang/ASTMatchers/ASTMatchers.h
1599

This is a bit *too* brief. It would be good to give some example code.

1608

This code does not look legal to me because of the X:. (Same for the Example match: text.)

1612

Did clang-format produce this formatting?

1622

I think it makes more sense to extend hasAnySubstatement() to work off stmtExpr() in addition to compoundStmt(). Same may be true for statementCountIs(), if you need it.

3013

Missing code example of how to use this and what would match.

3499

It would be good to add a new code example as to when this would trigger for an OpaqueValueExpr.

3568

Code examples for how this affects binaryConditionalOperator() (here and below) would be welcome.

a.sidorin updated this revision to Diff 48503.Feb 19 2016, 9:22 AM
a.sidorin edited edge metadata.
a.sidorin removed rL LLVM as the repository for this revision.

Fix issues pointed by Aaron.

aaron.ballman added inline comments.Feb 22 2016, 7:06 AM
include/clang/ASTMatchers/ASTMatchers.h
1000

Can remove the extra newline.

1005

I don't think this code is valid because of the }; and the missing semicolon at the end.

1019

It may be good to have an example of how this differs from ParenExpr with a list. e.g., I don't think this will match: int a = 0, b = 1; int i = (a, b);, will it? Either way, it would be good to have as an example (and possibly test case).

1646

Comment doesn't match code, should either match a ?: b or change the code to use b instead of c.

1648

Missing semicolon?

1654

This is still a bit too brief, because opaque value expressions aren't very commonly used, it's hard to understand what it will actually match. If you can expand the description a bit as to what an opaque value expression actually is, that would be great.

1658

Missing semicolon?

1820

Missing a period at the end of the sentence.

1822

Spurious ), I hope. ;-)

1830

Missing period.

1846

Neither of these look to be valid code. Both seem to have extra };, and the last one is missing a semicolon.

3506

Is this formatting that clang-format produced? The * looks to be in the wrong place.

LegalizeAdulthood added inline comments.
docs/LibASTMatchersReference.html
512

Can we put gnu in the name of matchers that only match GNU extensions?

544

Please provide an example here.

549

See above re: gnu specific extensions. When I read the name of this matcher, I expected it to be matching ==, !=, <=, etc.

1120

The docs say that OpaqueValueExpr doesn't correspond to concrete syntax, so why would we have a matcher for it?

1140

Can we have a simpler example? For instance, I can't see that ParenListExpr has anything to do with templates.

1149

Does it actually match the closing paren?

1175

Ditto re: gnu extension

1752

Please provide an example.

jbcoe added a subscriber: jbcoe.Mar 1 2016, 12:24 PM
a.sidorin updated this revision to Diff 49727.Mar 3 2016, 5:07 AM
a.sidorin marked 15 inline comments as done.

Add more comments; resolve issues pointed on review.

aaron.ballman added inline comments.Mar 7 2016, 6:31 AM
docs/LibASTMatchersReference.html
512

We name matchers after the AST node they represent. If we change the name of the AST node to include GNU, then I think it would make sense. I'm not certain whether we want to rename the AST nodes though (it would be a larger discussion than this review), so I would say we shouldn't put gnu in the matcher names, but call it out in the documentation (as the author is now doing).

include/clang/ASTMatchers/ASTMatchers.h
1012

late parsing instead of latter parsing?

1013

In the final AST (insert the).

1022

Combine the two examples into one. e.g.,

template<typename T> class X {
  void f() {
    X x(*this);
    int a = 0, b = 1; int i = (a, b);
   }
};
1606

Matches a statement expression (GNU extension).

(This way we can consistently search for GNU extension.)

a.sidorin updated this revision to Diff 50119.Mar 9 2016, 3:57 AM

Improved doxygen comments.

aaron.ballman accepted this revision.Mar 9 2016, 7:13 AM
aaron.ballman edited edge metadata.

LGTM, thank you!

This revision is now accepted and ready to land.Mar 9 2016, 7:13 AM
aaron.ballman closed this revision.Mar 9 2016, 9:16 AM

Commit in r263027.