Here you go.
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
