On Thu, Aug 16, 2012 at 1:20 AM, Manuel Klimek <[email protected]> wrote:
> > 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 ... > That's even stranger...but thankfully, I have commit access now. Committed, r162025. -Sam > > > > >> > >> > >> 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
