Hi Sam,<div><br></div><div>thanks for the patches!</div><div>It is definitely useful to separate the patches. In future, please also send them in different mails.</div><div><br></div><div>anyOf.patch: I removed some newlines and submitted as r160233.</div>
<div><br></div><div>decls.patch:</div><div><div><br></div><div> +/// declarationStatement(hasSingleDecl(anything()))<br>
</div><div> +/// matches c, but not a or b.</div></div><div><br></div><div>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.</div>
<div><br></div><div><div>+/// declCountIs(2)</div><div>+/// matches 'int a,b;' and 'int d = 2, e', but not 'int c'.</div></div><div><br></div><div>nit: Can you add a space after "a,"?<br>
</div><div><br></div><div><div> +TEST(SingleDecl, IsSingleDecl) {</div><div> + StatementMatcher SingleInitDecl =</div><div> + declarationStatement(hasSingleDecl(variable(hasInitializer(anything()))));</div><div>
+ EXPECT_TRUE(matches("void f() {for(int a = 4;;);}", SingleInitDecl));</div>
<div> + EXPECT_TRUE(notMatches("void f() {for(int a = 4, b = 3;;);}",</div><div> + SingleInitDecl));</div><div> +}</div></div><div><br></div><div>Maybe rename SingleInitDecl to SignelDeclStmt. Also, is there a reason for the for loop in the examples?</div>
<div><br></div><div>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?</div>
<div><br></div><div>ignore-casts.patch:</div><div>First of, I think these methods are going to be really helpful!</div><div><br></div><div>I am not sure about the naming. I am not a native speaker, but:</div><div><div> variable(hasInitializer(ignoreImplicitCasts(expression())))</div>
</div><div>feels a bit "rough". How about:</div><div><div> variable(hasInitializer(ignoringImplicitCasts(expression())))</div></div><div>Still does not feel ideal. Any thoughts?</div><div><br></div><div>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.</div>
<div><br></div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Jul 13, 2012 at 8:34 PM, Sam Panzer <span dir="ltr"><<a href="mailto:panzer@google.com" target="_blank">panzer@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>Attached are three more small matcher patches. One fixes another<br>
rename typo (AnyOf --> anyOf) that was similar to the previous<br>
allOf patch. The second patch adds more inspection for<br>
declarationStatement matchers, making it easier to look at single<br>
declarations directly. The third patch adds expression matchers which<br>
call IgnoreXXXCasts() before applying their<br>
sub-matchers.</div><div><br></div>For future reference, should I<br>
continue splitting up these patches for<br>
review?<div><br></div><div>-Sam</div><br>
</blockquote></div><br></div>