[cfe-commits] [PATCH] x3 More matcher patches

[Daniel Jasper]

Hi Sam,

thanks for the patches!
It is definitely useful to separate the patches. In future, please also
send them in different mails.

anyOf.patch: I removed some newlines and submitted as r160233.

decls.patch:

  +/// declarationStatement(hasSingleDecl(anything()))
  +///   matches c, but not a or b.

This is wrong. The declaration statement matches "int c;" and not "int a,
b;". This might seem light a nitpick, but it is important to be precise
with the very limited documentation that we provide.

+/// declCountIs(2)
+///   matches 'int a,b;' and 'int d = 2, e', but not 'int c'.

nit: Can you add a space after "a,"?

  +TEST(SingleDecl, IsSingleDecl) {
  +  StatementMatcher SingleInitDecl =
  +
 declarationStatement(hasSingleDecl(variable(hasInitializer(anything()))));
  +  EXPECT_TRUE(matches("void f() {for(int a = 4;;);}", SingleInitDecl));
  +  EXPECT_TRUE(notMatches("void f() {for(int a = 4, b = 3;;);}",
  +                          SingleInitDecl));
  +}

Maybe rename SingleInitDecl to SignelDeclStmt. Also, is there a reason for
the for loop in the examples?

In general, I think a matcher to match the Decl at location N of a DeclStmt
would be the next thing that might be needed (probably called "hasDecl").
Would you mind adding that to this patch?

ignore-casts.patch:
First of, I think these methods are going to be really helpful!

I am not sure about the naming. I am not a native speaker, but:
  variable(hasInitializer(ignoreImplicitCasts(expression())))
feels a bit "rough". How about:
  variable(hasInitializer(ignoringImplicitCasts(expression())))
Still does not feel ideal. Any thoughts?

The implementation definitely needs more elaborate comments with examples
of what the matchers are supposed to match. And the tests should also
include these examples with both positive and negative cases. Also, I think
you misunderstand what the ignoreParenCasts is mainly supposed to do. A
ParenExpr is added if you put parenthesis around an expression. Thus, the
ignoreParenCasts-method mainly handles cases like "int i = (0);". It also
strips of casts and thus the CStyleCastExpr in "const int y = (const int)
x;". This needs documentation and tests.



On Fri, Jul 13, 2012 at 8:34 PM, Sam Panzer <panzer at google.com> wrote:

> <div>Attached are three more small matcher patches. One fixes another
> rename typo (AnyOf --> anyOf) that was similar to the previous
> allOf patch. The second patch adds more inspection for
> declarationStatement matchers, making it easier to look at single
> declarations directly. The third patch adds expression matchers which
> call IgnoreXXXCasts() before  applying their
> sub-matchers.</div><div><br></div>For future reference, should I
> continue splitting up these patches for
> review?<div><br></div><div>-Sam</div>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120715/0753b5d9/attachment.html>