<div>+/// \brief Matches expressions that match InnerMatcher after any implicit casts</div><div>+/// are stripped off. Parentheses and explicit casts are not discarded.</div><div>+/// Given</div><div><br></div><div>Two line breaks after "stripped off.". As far as I know, the brief-comment should always be a single, reasonably short sentence. Same for the next comment.</div>
<div><br></div><div><div>+/// \brief Matches expressions that match InnerMatcher after parentheses</div><div>+/// are stripped off. Implicit and explicit casts are not discarded.</div><div><br></div><div>The second sentence IMO is the opposite of true :-).</div>
<div><br></div><div>+/// Given</div><div>+/// int a = (0);</div><div>+/// char b = 0;</div><div>+/// char c = (0);</div><div>+/// char d = (char)0;</div><div>+/// The matcher</div><div>+/// variable(hasInitializer(ignoringParenCasts(integerLiteral(0))))</div>
<div>+/// would match the declarations for a, b, and c, but not d.</div></div><div><br></div><div>From the description of Expr::IngoreParenCasts(), I would assume it does match the declaration of d as well. That seems to be the main difference between IgnoreParenCasts() and IgnoreParenImpCasts(). Also there seems to be a test supporting me ;-) (TEST(IgnoringParenCasts, MatchesOnlyWithParenCasts)).</div>
<div><br></div><div><div>+const internal::VariadicDynCastAllOfMatcher<</div><div>+ Stmt,</div><div>+ MaterializeTemporaryExpr> materializeTemporaryExpression;</div></div><div><br></div><div>Did you add this to this patch intentionally? Does not seem to be related and there are no tests for it.</div>
<div><br></div><div><div>+TEST(IgnoringImplicitCasts, DoesNotMatchIncorrectly) {</div><div>+ EXPECT_TRUE(notMatches("int x; const int y = x;",</div><div>+ variable(hasInitializer(ignoringImplicitCasts(</div>
<div>+ integerLiteral(equals(0)))))));</div></div><div><br></div><div>This (and some others like it) are somewhat useless tests with regard to the cast expressions. There is no way that the matcher could ever match something in the given source code, independent of whether the cast-matcher does or does not do what it is supposed to do. The only thing you might catch is that it always returns true. If you want to test that, make the inner part: unless(anything()).</div>
<div><br></div><div><div>+ EXPECT_TRUE(notMatches(</div><div>+ "int x = 0; float *y = (reinterpret_cast<float *>(&x));",</div><div>+ variable(hasInitializer(ignoringParenCasts(declarationReference())))));</div>
</div><div><br></div><div>I think, this might be confusing. If I understand it right, the test fails, because the declarationReference is child of a unaryOperator ("&"). The IgnoreParenCasts would happily remove the reinterpret_cast.</div>
<div><br></div><div><div>+ EXPECT_TRUE(notMatches("int x = 0; float *y = reinterpret_cast<float*>(&x);",</div><div>+ variable(hasInitializer(ignoringParenAndImplicitCasts(</div><div>
+ declarationReference())))));</div></div><div><br></div><div>Same as above. These tests are not only confusing, but also not essentially helpful. This test would still pass (i.e. the operator not match) if ignoreParenAndImplicitCasts() would suddenly "eat" the reinterpret_cast as the matcher does not match on the unaryOperator inside it.</div>
<div><br></div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jul 24, 2012 at 12:26 AM, 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">
<br><div class="gmail_extra">This thread forked from a previous group of patches, now with a new and improved version of additional cast-related matchers.<br><br><div class="gmail_quote">On Sun, Jul 15, 2012 at 1:44 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi Sam,<div><br></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> variable(hasInitializer(ignoreImplicitCasts(expression())))</div>
<div>feels a bit "rough". How about:</div><div> variable(hasInitializer(ignoringImplicitCasts(expression())))</div><div>Still does not feel ideal. Any thoughts?</div><div><br></div></blockquote><div> </div><div>
I went with "ignoringImplicitCasts" as suggested though "withoutImplicitCasts" could also work.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div></div><div>The implementation definitely needs more elaborate comments with examples of what the matchers are supposed to match.</div></blockquote><div><br></div><div>I added much more in-depth comments, complete with examples that differentiate the different kinds of casts involved here. It almost seems that I'm duplicating some AST documentation though, since these matchers just forward a method call to the inner matcher. In this case, ignoringParenCasts does exactly what Expr::IgnoreParenCasts() is supposed to do.</div>
<div>This made me wonder if it would be reasonable to create a macro that makes writing this kind of trivial matcher easier. I imagine something like this:</div><div><br></div><div> AST_MATCHER_METHOD(Expr, ignoringParenAndImplicitCasts, internal::Matcher<Expr>, IgnoreParenImpCasts);</div>
<div>which expands to the same result as</div><div> AST_MATCHER_P(Expr, ignoringParenAndImplicitCasts, internal::Matcher<Expr>, InnerMatcher) {</div><div> return InnerMatcher.matches(*Node.IgnoreParenImpCasts(), Finder, Builder);</div>
<div> }</div><div><br></div><div>Though this would only be useful if there are a good number of accessors to add, though I found around 15 existing matchers that just forwarded one method call and about 15 more matchers test the result of a method call for NULL before passing it on to an inner matcher. I expect that including a full predicate would be overkill, though.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">And the tests should also include these examples with both positive and negative cases.</blockquote>
<div><br></div><div>Tests added!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div>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><div><br></div></div></blockquote><div><br></div><div>You're right: I didn't realize that ignoreParenCasts meant stripping parentheses. This has been fixed :)</div><div><br></div><div>I also added a CastExpression matcher and changed the hasSourceExpression matcher to require any CastExpression, rather than an ImplicitCastExpression, as I wanted to use it to match an explicit cast to any integer type (since some for loop authors cast to int rather than using unsigned loop indices).</div>
<div><br></div></div><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div class="gmail_extra"><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>
</div></div></blockquote></div><br></div>
</blockquote></div><br></div>