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

Sam Panzer panzer at google.com
Thu Aug 16 09:59:27 PDT 2012


On Thu, Aug 16, 2012 at 1:20 AM, Manuel Klimek <klimek at google.com> wrote:

>
> 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 ...
>

That's even stranger...but thankfully, I have commit access now.
Committed, r162025.

-Sam


>
> >
> >>
> >>
> >> 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.
> >> >> >> >
> >> >> >> >
> >> >> >> >
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120816/dfa7f15a/attachment.html>


More information about the cfe-commits mailing list