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
