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 :) I'm happy to submit it, but patch complains about the patch - is there a reason there are no newlines after the @@ ... @@ parts? 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
