<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jul 25, 2012 at 7:11 AM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">As for the naming: I would leave it as is. Two reasons:<div>- We would also need to change the other one to ignoreParenAndImpCasts() and I dislike 5+ word method names. 5+ word matcher names will not make indentation of matcher-definitions any easier.</div>
<div>- We deviate from what people familiar with the AST are used to. I would prefer the matcher language being reasonably close to the corresponding AST entities/functions.</div></blockquote><div><br></div><div>Okay. For now, I'll leave the matcher as ignoreParenImplicitCasts() to parallel the AST; it can be changed later .</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div><br></div><div><div>+/// void* c = reinterpret_cast<char*>(&c);</div><div>+/// char d = char(0);</div><div>+/// The matcher</div><div>+/// variable(hasInitializer(ignoringParenCasts(integerLiteral())))</div>
<div>+/// would match the declarations for a, b, c, and d.</div></div><div><br></div><div>I strongly doubt it would match c's declaration.</div></blockquote><div><br></div><div>I strongly agree. This was supposed to be "void* c = reinterpret_cast<char*>(0);"</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div> /// class URL { URL(string); };</div>
<div> /// URL url = "a string";</div><div>-AST_MATCHER_P(ImplicitCastExpr, hasSourceExpression,</div><div>+AST_MATCHER_P(CastExpr, hasSourceExpression,</div><div><br></div></div><div>I think Manuel was quicker with this one. Should go away with a sync.</div>
<div><br></div></blockquote><div><br></div><div>It did.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div></div><div><div> return InnerMatcher.matches(NodeType, Finder, Builder);</div>
<div> }</div><div> </div><div>+</div><div> /// \brief Matches implicit casts whose destination type matches a given</div><div> /// matcher.</div>
</div><div><br></div><div>Please revert the newline...</div></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>
<br></div><div><div>+TEST(CastExpression, MatchesSimpleCases) {</div><div>+ EXPECT_TRUE(matches("char *p = reinterpret_cast<char *>(&p);",</div>
<div>+ expression(castExpression())));</div></div><div><br></div><div>I think just "castExpression()" instead of "expression(castExpression())" should be fine.</div></blockquote><div>
<br></div><div>Actually, without the expression() wrapper, I get a compiler error: (slightly abridged)</div><div><br></div><div><div>../tools/clang/unittests/ASTMatchers/ASTMatchersTest.h:57:10: error: call to member function 'addMatcher' is ambiguous</div>
<div> Finder.addMatcher(AMatcher, new VerifyMatch(0, &Found));</div></div><div><div>note: candidate function</div><div> void addMatcher(const DeclarationMatcher &NodeMatch,</div></div><div><div>note: candidate function</div>
<div> void addMatcher(const TypeMatcher &NodeMatch,</div></div><div><div>note: candidate function</div><div> void addMatcher(const StatementMatcher &NodeMatch,</div></div><div><br></div><div>I'm not sure what the correct fix is, but adding expression() makes the compiler happy. This test was modeled after the existing ReinterpretCast tests, which do the same thing :)<br>
</div><div>I wonder that it's related to the compiler error I received when trying to use id("name", allOf(...)), which I worked around by explicitly specifying the template parameter to id().</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><br></div>
<div><div>+ EXPECT_TRUE(matches("int x = 0; const int y = x;",</div><div>+ variable(hasInitializer(implicitCast(expression())))));</div></div><div><br></div><div>I think writing "implicitCast()" should lead to the same behavior as "implicitCast(expression())".</div>
<div><br></div></blockquote><div><br></div><div>Yep. Simplified as suggested.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div></div><div><div>+TEST(IgnoringParenCasts, MatchesOnlyWithParenCasts) {</div>
<div>+ EXPECT_TRUE(notMatches("int x = (0);",</div><div>+ variable(hasInitializer(integerLiteral(equals(0))))));</div>
</div><div><br></div><div>This EXPECT_TRUE(...) is duplicated (4 lines below).</div></blockquote><div><br></div><div>Fixed.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div>In general, it feels like the tests could be more focused. There are a lot of tests (which is good), but I find it very hard to tell, which cases are actually well-tested and what might still be untested. I think what might help is more documentation:</div>
<div><br></div><div>- Write for each test (not necessarily for each EXPECT_TRUE, but at least for each function), a one-sentence comment describing the purpose of this test. Example:</div><div><br></div><div>TEST(ImplicitCast, DoesNotMatchIncorrectly) {<br>
</div><div> // This tests ensures that implicitCast() only matches when a cast is present and</div><div> // ignores explicit casts.</div></blockquote><div><br></div><div>I tried to do this for the non-obvious tests (e.g. "MatchesSimpleCase" probably doesn't need such a comment). I think that one of the problems was the MatchesOnlyWithXXXCasts tests that were intended to check what happens with the addition of ignoringXXXCasts. The negative examples don't seem truly necessary, so I removed them and renamed the tests. The MatchesWithAndWithoutXXXCasts had a similar problem.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><br></div><div>When I wrote this comment, I actually noticed that there is no test case to test for paren-casts. Might be worthwhile to add one ("int x = (0);").</div>
<div><br></div></blockquote><div><br></div><div>Done. And a few more missing test cases were added (e.g. C-style casts in ignoringImplicitCasts).</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div></div><div>- For the non-obvious cases, explain the AST-structure that is actually being created. Example:</div><div><br></div></blockquote><div><br></div><div>I added comments notomg all the relevant implicit casts being created in these test cases.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div></div><div><div>TEST(IgnoringParenAndImpCasts, MatchesOnlyWithParenImpCasts) {</div><div> // The following tests create an implicit const-cast.</div>
<div> EXPECT_TRUE(notMatches("int x = 0; const int y = x;",</div><div> variable(hasInitializer(declarationReference()))));</div><div> EXPECT_TRUE(matches("int x = 0; const int y = x;",</div>
<div> variable(hasInitializer(ignoringParenAndImplicitCasts(</div><div> declarationReference())))));</div></div><div><br></div><div>I know this is very hand-waiving, but I hope you get the idea.</div>
</blockquote><div><br></div><div>I think it was clear enough to follow :)</div><div>How does this update look?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div>Cheers,<br>Daniel</div><div><div><div><br></div><div><br></div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jul 24, 2012 at 11:10 PM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@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><div>On Tue, Jul 24, 2012 at 9:03 PM, Sam Panzer <<a href="mailto:panzer@google.com" target="_blank">panzer@google.com</a>> wrote:<br>
><br>
><br>
><br>
> On Tue, Jul 24, 2012 at 1:12 AM, Daniel Jasper <<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>> wrote:<br>
>><br>
>> +/// \brief Matches expressions that match InnerMatcher after any implicit<br>
>> casts<br>
>> +/// are stripped off. Parentheses and explicit casts are not discarded.<br>
>> +/// Given<br>
>><br>
>> Two line breaks after "stripped off.". As far as I know, the brief-comment<br>
>> should always be a single, reasonably short sentence. Same for the next<br>
>> comment.<br>
><br>
><br>
> Done.<br>
><br>
>><br>
>><br>
>> +/// \brief Matches expressions that match InnerMatcher after parentheses<br>
>> +/// are stripped off. Implicit and explicit casts are not discarded.<br>
>><br>
>> The second sentence IMO is the opposite of true :-).<br>
><br>
><br>
> Fixed.<br>
><br>
>><br>
>><br>
>> +/// Given<br>
>> +/// int a = (0);<br>
>> +/// char b = 0;<br>
>> +/// char c = (0);<br>
>> +/// char d = (char)0;<br>
>> +/// The matcher<br>
>> +/// variable(hasInitializer(ignoringParenCasts(integerLiteral(0))))<br>
>> +/// would match the declarations for a, b, and c, but not d.<br>
>><br>
>> From the description of Expr::IngoreParenCasts(), I would assume it does<br>
>> match the declaration of d as well. That seems to be the main difference<br>
>> between IgnoreParenCasts() and IgnoreParenImpCasts(). Also there seems to be<br>
>> a test supporting me ;-) (TEST(IgnoringParenCasts,<br>
>> MatchesOnlyWithParenCasts)).<br>
><br>
><br>
> This comment was old and, yes, incorrect :) It's been fixed.<br>
><br>
>><br>
>><br>
>> +const internal::VariadicDynCastAllOfMatcher<<br>
>> + Stmt,<br>
>> + MaterializeTemporaryExpr> materializeTemporaryExpression;<br>
>><br>
>> Did you add this to this patch intentionally? Does not seem to be related<br>
>> and there are no tests for it.<br>
>><br>
><br>
> No, this belongs in a separate patch. I didn't correctly separate out new<br>
> matcher features this time...<br>
><br>
><br>
>><br>
>> +TEST(IgnoringImplicitCasts, DoesNotMatchIncorrectly) {<br>
>> + EXPECT_TRUE(notMatches("int x; const int y = x;",<br>
>> + variable(hasInitializer(ignoringImplicitCasts(<br>
>> + integerLiteral(equals(0)))))));<br>
>><br>
>> This (and some others like it) are somewhat useless tests with regard to<br>
>> the cast expressions. There is no way that the matcher could ever match<br>
>> something in the given source code, independent of whether the cast-matcher<br>
>> does or does not do what it is supposed to do. The only thing you might<br>
>> catch is that it always returns true. If you want to test that, make the<br>
>> inner part: unless(anything()).<br>
><br>
><br>
> The first tests in each DoesNotMatchIncorrectly pair was intended to check<br>
> the case where the inner matcher doesn't match, and the second test was to<br>
> check the chase where the inner matcher does match, but the cast expression<br>
> doesn't. As you point out, these are better expressed as unless(anything()).<br>
><br>
>><br>
>><br>
>> + EXPECT_TRUE(notMatches(<br>
>> + "int x = 0; float *y = (reinterpret_cast<float *>(&x));",<br>
>> +<br>
>> variable(hasInitializer(ignoringParenCasts(declarationReference())))));<br>
>><br>
>> I think, this might be confusing. If I understand it right, the test<br>
>> fails, because the declarationReference is child of a unaryOperator ("&").<br>
>> The IgnoreParenCasts would happily remove the reinterpret_cast.<br>
><br>
><br>
> I had this backwards - I thought IgnoreParenCasts was a subset of<br>
> IgnoreParenImpCasts, but it's really the other way around. Would it be a<br>
> good idea to rename ignoringParenCasts to ignoringParensAndCasts?<br>
<br>
</div></div>Spontaneous +1. This has caused me quite a few resets of my brain and<br>
wasted 2 minutes :)<br>
<div><div><br>
><br>
>><br>
>><br>
>> + EXPECT_TRUE(notMatches("int x = 0; float *y =<br>
>> reinterpret_cast<float*>(&x);",<br>
>> +<br>
>> variable(hasInitializer(ignoringParenAndImplicitCasts(<br>
>> + declarationReference())))));<br>
>><br>
>> Same as above. These tests are not only confusing, but also not<br>
>> essentially helpful. This test would still pass (i.e. the operator not<br>
>> match) if ignoreParenAndImplicitCasts() would suddenly "eat" the<br>
>> reinterpret_cast as the matcher does not match on the unaryOperator inside<br>
>> it.<br>
>><br>
><br>
> I cleaned up the tests. Are these better?<br>
><br>
>><br>
>><br>
>><br>
>> On Tue, Jul 24, 2012 at 12:26 AM, Sam Panzer <<a href="mailto:panzer@google.com" target="_blank">panzer@google.com</a>> wrote:<br>
>>><br>
>>><br>
>>> This thread forked from a previous group of patches, now with a new and<br>
>>> improved version of additional cast-related matchers.<br>
>>><br>
>>> On Sun, Jul 15, 2012 at 1:44 PM, Daniel Jasper <<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>><br>
>>> wrote:<br>
>>>><br>
>>>> Hi Sam,<br>
>>>><br>
>>>><br>
>>>> ignore-casts.patch:<br>
>>>> First of, I think these methods are going to be really helpful!<br>
>>>><br>
>>>> I am not sure about the naming. I am not a native speaker, but:<br>
>>>> variable(hasInitializer(ignoreImplicitCasts(expression())))<br>
>>>> feels a bit "rough". How about:<br>
>>>> variable(hasInitializer(ignoringImplicitCasts(expression())))<br>
>>>> Still does not feel ideal. Any thoughts?<br>
>>>><br>
>>><br>
>>> I went with "ignoringImplicitCasts" as suggested though<br>
>>> "withoutImplicitCasts" could also work.<br>
>>><br>
>>>><br>
>>>> The implementation definitely needs more elaborate comments with<br>
>>>> examples of what the matchers are supposed to match.<br>
>>><br>
>>><br>
>>> I added much more in-depth comments, complete with examples that<br>
>>> differentiate the different kinds of casts involved here. It almost seems<br>
>>> that I'm duplicating some AST documentation though, since these matchers<br>
>>> just forward a method call to the inner matcher. In this case,<br>
>>> ignoringParenCasts does exactly what Expr::IgnoreParenCasts() is supposed to<br>
>>> do.<br>
>>> This made me wonder if it would be reasonable to create a macro that<br>
>>> makes writing this kind of trivial matcher easier. I imagine something like<br>
>>> this:<br>
>>><br>
>>> AST_MATCHER_METHOD(Expr, ignoringParenAndImplicitCasts,<br>
>>> internal::Matcher<Expr>, IgnoreParenImpCasts);<br>
>>> which expands to the same result as<br>
>>> AST_MATCHER_P(Expr, ignoringParenAndImplicitCasts,<br>
>>> internal::Matcher<Expr>, InnerMatcher) {<br>
>>> return InnerMatcher.matches(*Node.IgnoreParenImpCasts(), Finder,<br>
>>> Builder);<br>
>>> }<br>
>>><br>
>>> Though this would only be useful if there are a good number of accessors<br>
>>> to add, though I found around 15 existing matchers that just forwarded one<br>
>>> method call and about 15 more matchers test the result of a method call for<br>
>>> NULL before passing it on to an inner matcher. I expect that including a<br>
>>> full predicate would be overkill, though.<br>
>>><br>
>>>><br>
>>>> And the tests should also include these examples with both positive and<br>
>>>> negative cases.<br>
>>><br>
>>><br>
>>> Tests added!<br>
>>><br>
>>>><br>
>>>> Also, I think you misunderstand what the ignoreParenCasts is mainly<br>
>>>> supposed to do. A ParenExpr is added if you put parenthesis around an<br>
>>>> expression. Thus, the ignoreParenCasts-method mainly handles cases like "int<br>
>>>> i = (0);". It also strips of casts and thus the CStyleCastExpr in "const int<br>
>>>> y = (const int) x;". This needs documentation and tests.<br>
>>>><br>
>>><br>
>>> You're right: I didn't realize that ignoreParenCasts meant stripping<br>
>>> parentheses. This has been fixed :)<br>
>>><br>
>>> I also added a CastExpression matcher and changed the hasSourceExpression<br>
>>> matcher to require any CastExpression, rather than an<br>
>>> ImplicitCastExpression, as I wanted to use it to match an explicit cast to<br>
>>> any integer type (since some for loop authors cast to int rather than using<br>
>>> unsigned loop indices).<br>
>>><br>
>>><br>
>>>><br>
>>>> On Fri, Jul 13, 2012 at 8:34 PM, Sam Panzer <<a href="mailto:panzer@google.com" target="_blank">panzer@google.com</a>> wrote:<br>
>>>>><br>
>>>>> <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>
>>>><br>
>>>><br>
>>><br>
>><br>
><br>
><br>
</div></div>> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
><br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>