On Mon, Aug 13, 2012 at 6:59 PM, Sam Panzer <[email protected]> wrote: > > > > 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.
Still the same problem ... > >> >> >> 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. >> >> >> > >> >> >> > >> >> >> > >> >> > >> >> > >> > >> > > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
