<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Aug 16, 2012 at 1:20 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"><div class="im"><br>
On Mon, Aug 13, 2012 at 6:59 PM, Sam Panzer <<a href="mailto:panzer@google.com">panzer@google.com</a>> wrote:<br>
><br>
><br>
><br>
> On Mon, Aug 13, 2012 at 1:57 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>><br>
>><br>
>> On Thu, Aug 2, 2012 at 7:48 PM, Sam Panzer <<a href="mailto:panzer@google.com">panzer@google.com</a>> wrote:<br>
>> > Here you go.<br>
>><br>
>> Sorry for not getting to this earlier - apparently nobody has<br>
>> submitted this yet :)<br>
><br>
><br>
> No problem :)<br>
><br>
>><br>
>> I'm happy to submit it, but patch complains about the patch - is there<br>
>> a reason there are no newlines after the @@ ... @@ parts?<br>
><br>
><br>
> That's odd...I must have deleted a spare newline in that section. Here's<br>
> another diff that I successfully patched onto master.<br>
<br>
</div>Still the same problem ...<br></blockquote><div><br></div><div>That's even stranger...but thankfully, I have commit access now.</div><div>Committed, r162025.</div><div><br></div><div>-Sam</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><div class="h5"><br>
><br>
>><br>
>><br>
>> Cheers,<br>
>> /Manuel<br>
>><br>
>> ><br>
>> ><br>
>> > On Thu, Aug 2, 2012 at 7:04 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>> >><br>
>> >><br>
>> >> A couple more things:<br>
>> >> - please completely take out the type() matcher - I've looked into it,<br>
>> >> and we need to come up with a good design here first; until then I<br>
>> >> don't think it makes sense to have a hack in<br>
>> >> - please rename castExpression -> castExpr to be consistent with the<br>
>> >> node<br>
>> >> name<br>
>> >><br>
>> >> Apart from that lgtm<br>
>> >><br>
>> >> Cheers,<br>
>> >> /Manuel<br>
>> >><br>
>> >> On Mon, Jul 30, 2012 at 7:49 PM, Sam Panzer <<a href="mailto:panzer@google.com">panzer@google.com</a>> wrote:<br>
>> >> ><br>
>> >> > A new version, with shorter (Implicit --> Imp) matcher names!<br>
>> >> ><br>
>> >> > On Mon, Jul 30, 2012 at 5:32 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>><br>
>> >> > wrote:<br>
>> >> >><br>
>> >> >><br>
>> >> >> On Thu, Jul 26, 2012 at 11:15 PM, Sam Panzer <<a href="mailto:panzer@google.com">panzer@google.com</a>><br>
>> >> >> wrote:<br>
>> >> >> ><br>
>> >> >> > Here's the next version!<br>
>> >> >> ><br>
>> >> >> > On Thu, Jul 26, 2012 at 1:53 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>><br>
>> >> >> > wrote:<br>
>> >> >> >><br>
>> >> >> >><br>
>> >> >> >> +/// \brief Matches expressions that match InnerMatcher after<br>
>> >> >> >> parentheses<br>
>> >> >> >> and<br>
>> >> >> >> +/// casts are stripped off.<br>
>> >> >> >> +///<br>
>> >> >> >> +/// Implicit and non-C Style casts are not discarded.<br>
>> >> >> >><br>
>> >> >> >> This contradicts the example...<br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> > s/not/also/<br>
>> >> >> > Though I'm not sure if this makes it any clearer than not having a<br>
>> >> >> > comment.<br>
>> >> >> ><br>
>> >> >> >><br>
>> >> >> >><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>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> > Actually, this comment wasn't correct either, since parentheses<br>
>> >> >> > aren't<br>
>> >> >> > CastExpr's. Fixed!<br>
>> >> >> ><br>
>> >> >> >><br>
>> >> >> >><br>
>> >> >> >> I'd vote for calling all new matchers we write exactly like the<br>
>> >> >> >> AST<br>
>> >> >> >> nodes (castExpr in this case).<br>
>> >> >> >> I lost the fight, and if we ever want to get to the place where<br>
>> >> >> >> it<br>
>> >> >> >> all<br>
>> >> >> >> just works, we have to start not introducing new violations of<br>
>> >> >> >> that<br>
>> >> >> >> rule.<br>
>> >> >> >> Same for Implicit -> Imp (it hurts me, but, oh well, I'll live ;)<br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> > I would agree on the AST-style naming convention. I can see three<br>
>> >> >> > naming<br>
>> >> >> > styles for CastExpr among existing matchers: cast (already<br>
>> >> >> > taken!),<br>
>> >> >> > castExpression (what I used), and castExpr. Which cast matchers<br>
>> >> >> > are<br>
>> >> >> > you<br>
>> >> >> > suggesting should be renamed, and to what? I'll be happy to change<br>
>> >> >> > them,<br>
>> >> >> > but<br>
>> >> >> > I'm not sure what you're asking.<br>
>> >> >><br>
>> >> >> ignoringImplicitCasts -> ignoringImpCasts<br>
>> >> >> ignoringParenImplicitCasts -> ingoringParenImpCasts<br>
>> >> ><br>
>> >> ><br>
>> >> > Done.<br>
>> >> ><br>
>> >> >><br>
>> >> >><br>
>> >> >> >> + EXPECT_TRUE(matches("char *p = reinterpret_cast<char *>(&p);",<br>
>> >> >> >> + expression(castExpression())));<br>
>> >> >> >><br>
>> >> >> >> Btw, if we defined the castExpr as<br>
>> >> >> >> 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<br>
>> >> >> >> use<br>
>> >> >> >> of<br>
>> >> >> >> castExpr.<br>
>> >> >> >><br>
>> >> >> ><br>
>> >> >> > I was following the other cast examples such as implicitCast.<br>
>> >> >> > Would<br>
>> >> >> > it<br>
>> >> >> > be<br>
>> >> >> > better to change this?<br>
>> >> >><br>
>> >> >> Yes, and I think we should change the others, but not in this CL;<br>
>> >> >> just<br>
>> >> >> use<br>
>> >> >> const internal::VariadicDynCastAllOfMatcher<Stmt, CastExpr><br>
>> >> >> castExpression;<br>
>> >> >><br>
>> >> >> ><br>
>> >> >> >><br>
>> >> >> >> +<br>
>> >> >> >> pointsTo(TypeMatcher(anything())))))));<br>
>> >> >> >><br>
>> >> >> >> This is unfortunate. We should add a type() matcher.<br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> > I've used this workaround in my client code - and I just<br>
>> >> >> > discovered<br>
>> >> >> > that<br>
>> >> >> > the<br>
>> >> >> > test Matcher.HandlesNullQualTypes does too (with a fixme echoing<br>
>> >> >> > your<br>
>> >> >> > complaint):<br>
>> >> >> > const TypeMatcher AnyType = anything();<br>
>> >> >> ><br>
>> >> >> > I briefly tried adding a type() matcher, but it seems like<br>
>> >> >> > matchers<br>
>> >> >> > treat<br>
>> >> >> > types differently from statements and declarations. All existing<br>
>> >> >> > matchers on<br>
>> >> >> > types just take a single argument, which is usually<br>
>> >> >> > hasDeclaration()<br>
>> >> >> > in<br>
>> >> >> > some<br>
>> >> >> > form (often indirectly), so it's not as easy as adding another<br>
>> >> >> > AST_MATCHER<br>
>> >> >> > definition. I'll leave this up to someone who really knows how the<br>
>> >> >> > new<br>
>> >> >> > matcher should fit in :)<br>
>> >> >><br>
>> >> >> A type matcher would be added like this:<br>
>> >> >> const internal::VariadicDynCastAllOfMatcher<QualType, QualtType><br>
>> >> >> type;<br>
>> >> ><br>
>> >> ><br>
>> >> > I tried that already. It fails because QualType does not define the<br>
>> >> > member<br>
>> >> > functions needed for dyn_cast to work, i.e. classOf(). I'm not brave<br>
>> >> > enough<br>
>> >> > to try modifying QualType directly just for a type matcher, nor am I<br>
>> >> > sure<br>
>> >> > that it's the right fix.<br>
>> >> ><br>
>> >> >><br>
>> >> >><br>
>> >> >> >> +TEST(CastExpression, MatchesSimpleCases) {<br>
>> >> >> >><br>
>> >> >> >> I'm not really happy with those names, but I'm aware it's really<br>
>> >> >> >> hard<br>
>> >> >> >> to come up with good test names in those cases.<br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> > I broke the test into MatchesImplicitCasts and<br>
>> >> >> > MatchesExplicitCasts,<br>
>> >> >> > both of<br>
>> >> >> > which are much better names.<br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> ><br>
>> >> ><br>
>> >> ><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div>