[PATCH] Add caseStmt(), defaultStmt(), eachCase() and hasCaseConstant() matchers.
Peter Collingbourne
peter at pcc.me.uk
Wed May 8 15:33:01 PDT 2013
================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:3356
@@ +3355,3 @@
+/// statement.
+AST_MATCHER_P(SwitchStmt, eachCase, internal::Matcher<SwitchCase>,
+ InnerMatcher) {
----------------
Daniel Jasper wrote:
> I am not very happy with the naming here:
> 1) This should follow the conventions we already have. Meaning, this should be called forEach.. Ideally, we would then also add a corresponding has...
> 2) It is slightly weird, that this is called ..Case, when in fact it matches on SwitchCases. I'd vote for either calling this ..SwitchCase or only matching on CaseStmts.
Renamed.
================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:3378
@@ +3377,3 @@
+/// the GNU case range extension, matches the constant given in the case
+/// statement.
+AST_MATCHER_P(SwitchCase, hasCaseConstant, internal::Matcher<Expr>,
----------------
Daniel Jasper wrote:
> Please add a usage example.
Done.
================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:3379
@@ +3378,3 @@
+/// statement.
+AST_MATCHER_P(SwitchCase, hasCaseConstant, internal::Matcher<Expr>,
+ InnerMatcher) {
----------------
Daniel Jasper wrote:
> I think this should just be a matcher on CaseStmt (making the dyn_cast) unnecessary. Or am I missing something?
I initially wrote this matcher this way so that users could write forEachSwitchCase(hasCaseConstant()) (which is already quite verbose) without also using caseStmt(). But I guess this would make it consistent with other matchers. Changed.
================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:2922
@@ +2921,3 @@
+ EXPECT_TRUE(
+ notMatches("void x() { switch(42); }", switchStmt(eachCase(caseStmt()))));
+ EXPECT_TRUE(matches("void x() { switch(42) case 42:; }",
----------------
Daniel Jasper wrote:
> There are no tests that verify that "eachCase" binds all matches. We need something like:
>
> EXPECT_TRUE(matchAndVerifyResultTrue(
> "void x() { switch (42) { case 1: case 2: break; case 3: default: break } }",
> switchStmt(eachCase(caseStmt().bind("x"))),
> new VerifyIdIsBoundTo<CaseStmt>("x", 3)));
I added something like this.
================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:3355
@@ +3354,3 @@
+/// \brief Matches each case or default statement belonging to the given switch
+/// statement.
+AST_MATCHER_P(SwitchStmt, eachCase, internal::Matcher<SwitchCase>,
----------------
Daniel Jasper wrote:
> Please add a usage example (like all the other matchers have).
Done.
http://llvm-reviews.chandlerc.com/D744
More information about the cfe-commits
mailing list