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

[Manuel Klimek]

On Mon, Aug 13, 2012 at 6:59 PM, Sam Panzer <panzer at google.com> wrote:
>
>
>
> On Mon, Aug 13, 2012 at 1:57 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>>
>> On Thu, Aug 2, 2012 at 7:48 PM, Sam Panzer <panzer at google.com> wrote:
>> > Here you go.
>>
>> Sorry for not getting to this earlier - apparently nobody has
>> submitted this yet :)
>
>
> No problem :)
>
>>
>> I'm happy to submit it, but patch complains about the patch - is there
>> a reason there are no newlines after the @@ ... @@ parts?
>
>
> That's odd...I must have deleted a spare newline in that section. Here's
> another diff that I successfully patched onto master.

Still the same problem ...

>
>>
>>
>> Cheers,
>> /Manuel
>>
>> >
>> >
>> > On Thu, Aug 2, 2012 at 7:04 AM, Manuel Klimek <klimek at google.com> wrote:
>> >>
>> >>
>> >> 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.
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>