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

[Sam Panzer]

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/20120730/a8cc5ef7/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cast-matchers-update-3.patch
Type: application/octet-stream
Size: 15345 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120730/a8cc5ef7/attachment.obj>