On Mon, Aug 13, 2012 at 1:57 AM, Manuel Klimek <[email protected]> wrote:
> > On Thu, Aug 2, 2012 at 7:48 PM, Sam Panzer <[email protected]> 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. > > Cheers, > /Manuel > > > > > > > On Thu, Aug 2, 2012 at 7:04 AM, Manuel Klimek <[email protected]> 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 <[email protected]> wrote: > >> > > >> > A new version, with shorter (Implicit --> Imp) matcher names! > >> > > >> > On Mon, Jul 30, 2012 at 5:32 AM, Manuel Klimek <[email protected]> > >> > wrote: > >> >> > >> >> > >> >> On Thu, Jul 26, 2012 at 11:15 PM, Sam Panzer <[email protected]> > wrote: > >> >> > > >> >> > Here's the next version! > >> >> > > >> >> > On Thu, Jul 26, 2012 at 1:53 AM, Manuel Klimek <[email protected]> > >> >> > 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. > >> >> > > >> >> > > >> >> > > >> > > >> > > > > > >
cast-matchers-update-4.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
