<br><div class="gmail_extra">A new version, with shorter (Implicit --> Imp) matcher names!<br><br><div class="gmail_quote">On Mon, Jul 30, 2012 at 5:32 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="HOEnZb"><div class="h5"><br>
On Thu, Jul 26, 2012 at 11:15 PM, Sam Panzer <<a href="mailto:panzer@google.com">panzer@google.com</a>> 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>> wrote:<br>
>><br>
>><br>
>> +/// \brief Matches expressions that match InnerMatcher after 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 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 aren't<br>
> CastExpr's. Fixed!<br>
><br>
>><br>
>><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>
><br>
><br>
> I would agree on the AST-style naming convention. I can see three naming<br>
> styles for CastExpr among existing matchers: cast (already taken!),<br>
> castExpression (what I used), and castExpr. Which cast matchers are you<br>
> suggesting should be renamed, and to what? I'll be happy to change them, but<br>
> I'm not sure what you're asking.<br>
<br>
</div></div>ignoringImplicitCasts -> ignoringImpCasts<br>
ignoringParenImplicitCasts -> ingoringParenImpCasts<br></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 class="im"><br>
>> + EXPECT_TRUE(matches("char *p = reinterpret_cast<char *>(&p);",<br>
>> + expression(castExpression())));<br>
>><br>
>> 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>
><br>
> I was following the other cast examples such as implicitCast. Would it be<br>
> better to change this?<br>
<br>
</div>Yes, and I think we should change the others, but not in this CL; just use<br>
const internal::VariadicDynCastAllOfMatcher<Stmt, CastExpr> castExpression;<br>
<div class="im"><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 discovered that the<br>
> test Matcher.HandlesNullQualTypes does too (with a fixme echoing your<br>
> complaint):<br>
> const TypeMatcher AnyType = anything();<br>
><br>
> I briefly tried adding a type() matcher, but it seems like matchers treat<br>
> types differently from statements and declarations. All existing matchers on<br>
> types just take a single argument, which is usually hasDeclaration() in some<br>
> form (often indirectly), so it's not as easy as adding another AST_MATCHER<br>
> definition. I'll leave this up to someone who really knows how the new<br>
> matcher should fit in :)<br>
<br>
</div>A type matcher would be added like this:<br>
const internal::VariadicDynCastAllOfMatcher<QualType, QualtType> type;<br></blockquote><div><br></div><div>I tried that already. It fails because QualType does not define the member functions needed for dyn_cast to work, i.e. classOf(). I'm not brave enough to try modifying QualType directly just for a type matcher, nor am I sure that it's the right fix.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><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>
><br>
><br>
> I broke the test into MatchesImplicitCasts and MatchesExplicitCasts, both of<br>
> which are much better names.<br>
><br>
><br>
><br>
</div></div></blockquote></div><br></div>