<br><div class="gmail_extra">Here's the next version!<br><br><div class="gmail_quote">On Thu, Jul 26, 2012 at 1:53 AM, 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"><br>
+/// \brief Matches expressions that match InnerMatcher after parentheses and<br>
+/// casts are stripped off.<br>
+///<br>
+/// Implicit and non-C Style casts are not discarded.<br>
<br>
This contradicts the example...<br></blockquote><div><br></div><div>s/not/also/</div><div>Though I'm not sure if this makes it any clearer than not having a comment.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+/// \brief Matches any cast nodes of Clang's AST.<br>
+///<br>
+/// Example: castExpression() matches each of the following:<br>
+/// (int) 3;<br>
+/// const_cast<Expr *>(SubExpr);<br>
+/// (i);<br>
+/// char c = 0;<br>
+const internal::VariadicDynCastAllOfMatcher<<br>
+ Expr,<br>
+ CastExpr> castExpression;<br></blockquote><div><br></div><div>Actually, this comment wasn't correct either, since parentheses aren't CastExpr's. Fixed!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I'd vote for calling all new matchers we write exactly like the AST<br>
nodes (castExpr in this case).<br>
I lost the fight, and if we ever want to get to the place where it all<br>
just works, we have to start not introducing new violations of that<br>
rule.<br>
Same for Implicit -> Imp (it hurts me, but, oh well, I'll live ;)<br></blockquote><div><br></div><div>I would agree on the AST-style naming convention. I can see three naming styles for CastExpr among existing matchers: cast (already taken!), castExpression (what I used), and castExpr. Which cast matchers are you suggesting should be renamed, and to what? I'll be happy to change them, but I'm not sure what you're asking.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
+ EXPECT_TRUE(matches("char *p = reinterpret_cast<char *>(&p);",<br>
+ expression(castExpression())));<br>
<br>
</div>Btw, if we defined the castExpr as VariadicDynCastAllOfMatcher<Stmt,<br>
CastExpr> just putting them in top-level would work, too. Since a<br>
Matcher<Stmt> is-a Matcher<Expr> this would also not limit the use of<br>
castExpr.<br>
<br></blockquote><div><br></div><div>I was following the other cast examples such as implicitCast. Would it be better to change this?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ pointsTo(TypeMatcher(anything())))))));<br>
<br>
This is unfortunate. We should add a type() matcher.</blockquote><div> </div><div>I've used this workaround in my client code - and I just discovered that the test Matcher.HandlesNullQualTypes does too (with a fixme echoing your complaint): </div>
<div> const TypeMatcher AnyType = anything();<br></div><div><br></div><div>I briefly tried adding a type() matcher, but it seems like matchers treat types differently from statements and declarations. All existing matchers on types just take a single argument, which is usually hasDeclaration() in some form (often indirectly), so it's not as easy as adding another AST_MATCHER definition. I'll leave this up to someone who really knows how the new matcher should fit in :)</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
+TEST(CastExpression, MatchesSimpleCases) {<br>
<br>
I'm not really happy with those names, but I'm aware it's really hard<br>
to come up with good test names in those cases.<br></blockquote><div><br></div><div>I broke the test into MatchesImplicitCasts and MatchesExplicitCasts, both of which are much better names.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div><br>
On Wed, Jul 25, 2012 at 11:01 PM, Sam Panzer <<a href="mailto:panzer@google.com" target="_blank">panzer@google.com</a>> wrote:<br>
><br>
><br>
><br>
> On Wed, Jul 25, 2012 at 7:11 AM, Daniel Jasper <<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>> wrote:<br>
>><br>
>> As for the naming: I would leave it as is. Two reasons:<br>
>> - We would also need to change the other one to ignoreParenAndImpCasts()<br>
>> and I dislike 5+ word method names. 5+ word matcher names will not make<br>
>> indentation of matcher-definitions any easier.<br>
>> - We deviate from what people familiar with the AST are used to. I would<br>
>> prefer the matcher language being reasonably close to the corresponding AST<br>
>> entities/functions.<br>
><br>
><br>
> Okay. For now, I'll leave the matcher as ignoreParenImplicitCasts() to<br>
> parallel the AST; it can be changed later .<br>
><br>
>><br>
>><br>
>><br>
>> +/// void* c = reinterpret_cast<char*>(&c);<br>
>> +/// char d = char(0);<br>
>> +/// The matcher<br>
>> +/// variable(hasInitializer(ignoringParenCasts(integerLiteral())))<br>
>> +/// would match the declarations for a, b, c, and d.<br>
>><br>
>> I strongly doubt it would match c's declaration.<br>
><br>
><br>
> I strongly agree. This was supposed to be "void* c =<br>
> reinterpret_cast<char*>(0);"<br>
><br>
>> /// class URL { URL(string); };<br>
>> /// URL url = "a string";<br>
>> -AST_MATCHER_P(ImplicitCastExpr, hasSourceExpression,<br>
>> +AST_MATCHER_P(CastExpr, hasSourceExpression,<br>
>><br>
>> I think Manuel was quicker with this one. Should go away with a sync.<br>
>><br>
><br>
> It did.<br>
><br>
>><br>
>> return InnerMatcher.matches(NodeType, Finder, Builder);<br>
>> }<br>
>><br>
>> +<br>
>> /// \brief Matches implicit casts whose destination type matches a given<br>
>> /// matcher.<br>
>><br>
>> Please revert the newline...<br>
><br>
><br>
> Done.<br>
><br>
>><br>
>><br>
>> +TEST(CastExpression, MatchesSimpleCases) {<br>
>> + EXPECT_TRUE(matches("char *p = reinterpret_cast<char *>(&p);",<br>
>> + expression(castExpression())));<br>
>><br>
>> I think just "castExpression()" instead of "expression(castExpression())"<br>
>> should be fine.<br>
><br>
><br>
> Actually, without the expression() wrapper, I get a compiler error:<br>
> (slightly abridged)<br>
><br>
> ../tools/clang/unittests/ASTMatchers/ASTMatchersTest.h:57:10: error: call to<br>
> member function 'addMatcher' is ambiguous<br>
> Finder.addMatcher(AMatcher, new VerifyMatch(0, &Found));<br>
> note: candidate function<br>
> void addMatcher(const DeclarationMatcher &NodeMatch,<br>
> note: candidate function<br>
> void addMatcher(const TypeMatcher &NodeMatch,<br>
> note: candidate function<br>
> void addMatcher(const StatementMatcher &NodeMatch,<br>
><br>
> I'm not sure what the correct fix is, but adding expression() makes the<br>
> compiler happy. This test was modeled after the existing ReinterpretCast<br>
> tests, which do the same thing :)<br>
> I wonder that it's related to the compiler error I received when trying to<br>
> use id("name", allOf(...)), which I worked around by explicitly specifying<br>
> the template parameter to id().<br>
><br>
>><br>
>><br>
>> + EXPECT_TRUE(matches("int x = 0; const int y = x;",<br>
>> +<br>
>> variable(hasInitializer(implicitCast(expression())))));<br>
>><br>
>> I think writing "implicitCast()" should lead to the same behavior as<br>
>> "implicitCast(expression())".<br>
>><br>
><br>
> Yep. Simplified as suggested.<br>
><br>
>><br>
>> +TEST(IgnoringParenCasts, MatchesOnlyWithParenCasts) {<br>
>> + EXPECT_TRUE(notMatches("int x = (0);",<br>
>> +<br>
>> variable(hasInitializer(integerLiteral(equals(0))))));<br>
>><br>
>> This EXPECT_TRUE(...) is duplicated (4 lines below).<br>
><br>
><br>
> Fixed.<br>
><br>
>><br>
>><br>
>> In general, it feels like the tests could be more focused. There are a lot<br>
>> of tests (which is good), but I find it very hard to tell, which cases are<br>
>> actually well-tested and what might still be untested. I think what might<br>
>> help is more documentation:<br>
>><br>
>> - Write for each test (not necessarily for each EXPECT_TRUE, but at least<br>
>> for each function), a one-sentence comment describing the purpose of this<br>
>> test. Example:<br>
>><br>
>> TEST(ImplicitCast, DoesNotMatchIncorrectly) {<br>
>> // This tests ensures that implicitCast() only matches when a cast is<br>
>> present and<br>
>> // ignores explicit casts.<br>
><br>
><br>
> I tried to do this for the non-obvious tests (e.g. "MatchesSimpleCase"<br>
> probably doesn't need such a comment). I think that one of the problems was<br>
> the MatchesOnlyWithXXXCasts tests that were intended to check what happens<br>
> with the addition of ignoringXXXCasts. The negative examples don't seem<br>
> truly necessary, so I removed them and renamed the tests. The<br>
> MatchesWithAndWithoutXXXCasts had a similar problem.<br>
><br>
>><br>
>><br>
>> When I wrote this comment, I actually noticed that there is no test case<br>
>> to test for paren-casts. Might be worthwhile to add one ("int x = (0);").<br>
>><br>
><br>
> Done. And a few more missing test cases were added (e.g. C-style casts in<br>
> ignoringImplicitCasts).<br>
><br>
><br>
>><br>
>> - For the non-obvious cases, explain the AST-structure that is actually<br>
>> being created. Example:<br>
>><br>
><br>
> I added comments notomg all the relevant implicit casts being created in<br>
> these test cases.<br>
><br>
>><br>
>> TEST(IgnoringParenAndImpCasts, MatchesOnlyWithParenImpCasts) {<br>
>> // The following tests create an implicit const-cast.<br>
>> EXPECT_TRUE(notMatches("int x = 0; const int y = x;",<br>
>><br>
>> variable(hasInitializer(declarationReference()))));<br>
>> EXPECT_TRUE(matches("int x = 0; const int y = x;",<br>
>><br>
>> variable(hasInitializer(ignoringParenAndImplicitCasts(<br>
>> declarationReference())))));<br>
>><br>
>> I know this is very hand-waiving, but I hope you get the idea.<br>
><br>
><br>
> I think it was clear enough to follow :)<br>
> How does this update look?<br>
><br>
>><br>
>><br>
>> Cheers,<br>
>> Daniel<br>
>><br>
>><br>
>><br>
>><br>
>> On Tue, Jul 24, 2012 at 11:10 PM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:<br>
>>><br>
>>> 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>><br>
>>> > wrote:<br>
>>> >><br>
>>> >> +/// \brief Matches expressions that match InnerMatcher after any<br>
>>> >> implicit<br>
>>> >> casts<br>
>>> >> +/// are stripped off. Parentheses and explicit casts are not<br>
>>> >> discarded.<br>
>>> >> +/// Given<br>
>>> >><br>
>>> >> Two line breaks after "stripped off.". As far as I know, the<br>
>>> >> brief-comment<br>
>>> >> should always be a single, reasonably short sentence. Same for the<br>
>>> >> next<br>
>>> >> comment.<br>
>>> ><br>
>>> ><br>
>>> > Done.<br>
>>> ><br>
>>> >><br>
>>> >><br>
>>> >> +/// \brief Matches expressions that match InnerMatcher after<br>
>>> >> 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>
>>> >> +///<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<br>
>>> >> does<br>
>>> >> match the declaration of d as well. That seems to be the main<br>
>>> >> difference<br>
>>> >> between IgnoreParenCasts() and IgnoreParenImpCasts(). Also there seems<br>
>>> >> 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<br>
>>> >> 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<br>
>>> > new<br>
>>> > matcher features this time...<br>
>>> ><br>
>>> ><br>
>>> >><br>
>>> >> +TEST(IgnoringImplicitCasts, DoesNotMatchIncorrectly) {<br>
>>> >> + EXPECT_TRUE(notMatches("int x; const int y = x;",<br>
>>> >> +<br>
>>> >> variable(hasInitializer(ignoringImplicitCasts(<br>
>>> >> + integerLiteral(equals(0)))))));<br>
>>> >><br>
>>> >> This (and some others like it) are somewhat useless tests with regard<br>
>>> >> to<br>
>>> >> the cast expressions. There is no way that the matcher could ever<br>
>>> >> match<br>
>>> >> something in the given source code, independent of whether the<br>
>>> >> cast-matcher<br>
>>> >> does or does not do what it is supposed to do. The only thing you<br>
>>> >> might<br>
>>> >> catch is that it always returns true. If you want to test that, make<br>
>>> >> the<br>
>>> >> inner part: unless(anything()).<br>
>>> ><br>
>>> ><br>
>>> > The first tests in each DoesNotMatchIncorrectly pair was intended to<br>
>>> > check<br>
>>> > the case where the inner matcher doesn't match, and the second test was<br>
>>> > to<br>
>>> > check the chase where the inner matcher does match, but the cast<br>
>>> > expression<br>
>>> > doesn't. As you point out, these are better expressed as<br>
>>> > unless(anything()).<br>
>>> ><br>
>>> >><br>
>>> >><br>
>>> >> + EXPECT_TRUE(notMatches(<br>
>>> >> + "int x = 0; float *y = (reinterpret_cast<float *>(&x));",<br>
>>> >> +<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>
>>> >> ("&").<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<br>
>>> > a<br>
>>> > good idea to rename ignoringParenCasts to ignoringParensAndCasts?<br>
>>><br>
>>> Spontaneous +1. This has caused me quite a few resets of my brain and<br>
>>> wasted 2 minutes :)<br>
>>><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<br>
>>> >> 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>><br>
>>> >> wrote:<br>
>>> >>><br>
>>> >>><br>
>>> >>> This thread forked from a previous group of patches, now with a new<br>
>>> >>> 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<br>
>>> >>> seems<br>
>>> >>> that I'm duplicating some AST documentation though, since these<br>
>>> >>> matchers<br>
>>> >>> just forward a method call to the inner matcher. In this case,<br>
>>> >>> ignoringParenCasts does exactly what Expr::IgnoreParenCasts() is<br>
>>> >>> 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<br>
>>> >>> 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<br>
>>> >>> accessors<br>
>>> >>> to add, though I found around 15 existing matchers that just<br>
>>> >>> forwarded one<br>
>>> >>> method call and about 15 more matchers test the result of a method<br>
>>> >>> call for<br>
>>> >>> NULL before passing it on to an inner matcher. I expect that<br>
>>> >>> including a<br>
>>> >>> full predicate would be overkill, though.<br>
>>> >>><br>
>>> >>>><br>
>>> >>>> And the tests should also include these examples with both positive<br>
>>> >>>> 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<br>
>>> >>>> an<br>
>>> >>>> expression. Thus, the ignoreParenCasts-method mainly handles cases<br>
>>> >>>> like "int<br>
>>> >>>> i = (0);". It also strips of casts and thus the CStyleCastExpr in<br>
>>> >>>> "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<br>
>>> >>> hasSourceExpression<br>
>>> >>> matcher to require any CastExpression, rather than an<br>
>>> >>> ImplicitCastExpression, as I wanted to use it to match an explicit<br>
>>> >>> cast to<br>
>>> >>> any integer type (since some for loop authors cast to int rather than<br>
>>> >>> 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>><br>
>>> >>>> wrote:<br>
>>> >>>>><br>
>>> >>>>> <div>Attached are three more small matcher patches. One fixes<br>
>>> >>>>> 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<br>
>>> >>>>> 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>
>>> > _______________________________________________<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>
>><br>
>><br>
><br>
</div></div></blockquote></div><br></div>