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-3.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
