[cfe-commits] [PATCH]: Matchers for ignoring paren/implicit casts

[Manuel Klimek]

A couple more things:
- please completely take out the type() matcher - I've looked into it,
and we need to come up with a good design here first; until then I
don't think it makes sense to have a hack in
- please rename castExpression -> castExpr to be consistent with the node name

Apart from that lgtm

Cheers,
/Manuel

On Mon, Jul 30, 2012 at 7:49 PM, Sam Panzer <panzer at google.com> wrote:
>
> A new version, with shorter (Implicit --> Imp) matcher names!
>
> On Mon, Jul 30, 2012 at 5:32 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>>
>> On Thu, Jul 26, 2012 at 11:15 PM, Sam Panzer <panzer at google.com> wrote:
>> >
>> > Here's the next version!
>> >
>> > On Thu, Jul 26, 2012 at 1:53 AM, Manuel Klimek <klimek at google.com>
>> > wrote:
>> >>
>> >>
>> >> +/// \brief Matches expressions that match InnerMatcher after
>> >> parentheses
>> >> and
>> >> +/// casts are stripped off.
>> >> +///
>> >> +/// Implicit and non-C Style casts are not discarded.
>> >>
>> >> This contradicts the example...
>> >
>> >
>> > s/not/also/
>> > Though I'm not sure if this makes it any clearer than not having a
>> > comment.
>> >
>> >>
>> >>
>> >> +/// \brief Matches any cast nodes of Clang's AST.
>> >> +///
>> >> +/// Example: castExpression() matches each of the following:
>> >> +///   (int) 3;
>> >> +///   const_cast<Expr *>(SubExpr);
>> >> +///   (i);
>> >> +///   char c = 0;
>> >> +const internal::VariadicDynCastAllOfMatcher<
>> >> +  Expr,
>> >> +  CastExpr> castExpression;
>> >
>> >
>> > Actually, this comment wasn't correct either, since parentheses aren't
>> > CastExpr's. Fixed!
>> >
>> >>
>> >>
>> >> I'd vote for calling all new matchers we write exactly like the AST
>> >> nodes (castExpr in this case).
>> >> I lost the fight, and if we ever want to get to the place where it all
>> >> just works, we have to start not introducing new violations of that
>> >> rule.
>> >> Same for Implicit -> Imp (it hurts me, but, oh well, I'll live ;)
>> >
>> >
>> > 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.
>>
>> ignoringImplicitCasts -> ignoringImpCasts
>> ignoringParenImplicitCasts -> ingoringParenImpCasts
>
>
> Done.
>
>>
>>
>> >> +  EXPECT_TRUE(matches("char *p = reinterpret_cast<char *>(&p);",
>> >> +                      expression(castExpression())));
>> >>
>> >> Btw, if we defined the castExpr as VariadicDynCastAllOfMatcher<Stmt,
>> >> CastExpr> just putting them in top-level would work, too. Since a
>> >> Matcher<Stmt> is-a Matcher<Expr> this would also not limit the use of
>> >> castExpr.
>> >>
>> >
>> > I was following the other cast examples such as implicitCast. Would it
>> > be
>> > better to change this?
>>
>> Yes, and I think we should change the others, but not in this CL; just use
>> const internal::VariadicDynCastAllOfMatcher<Stmt, CastExpr>
>> castExpression;
>>
>> >
>> >>
>> >> +                          pointsTo(TypeMatcher(anything())))))));
>> >>
>> >> This is unfortunate. We should add a type() matcher.
>> >
>> >
>> > 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):
>> >   const TypeMatcher AnyType = anything();
>> >
>> > 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 :)
>>
>> A type matcher would be added like this:
>> const internal::VariadicDynCastAllOfMatcher<QualType, QualtType> type;
>
>
> 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.
>
>>
>>
>> >> +TEST(CastExpression, MatchesSimpleCases) {
>> >>
>> >> I'm not really happy with those names, but I'm aware it's really hard
>> >> to come up with good test names in those cases.
>> >
>> >
>> > I broke the test into MatchesImplicitCasts and MatchesExplicitCasts,
>> > both of
>> > which are much better names.
>> >
>> >
>> >
>
>