[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