[cfe-commits] [PATCH]: Matchers for ignoring paren/implicit casts
Manuel Klimek
klimek at google.com
Mon Jul 30 05:32:42 PDT 2012
On Thu, Jul 26, 2012 at 11:15 PM, Sam Panzer <panzer at google.com> wrote:
>
> Here's the next version!
>
> On Thu, Jul 26, 2012 at 1:53 AM, Manuel Klimek <klimek at google.com> 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
>> + 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;
>> +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.
>
>>
>>
>> On Wed, Jul 25, 2012 at 11:01 PM, Sam Panzer <panzer at google.com> wrote:
>> >
>> >
>> >
>> > On Wed, Jul 25, 2012 at 7:11 AM, Daniel Jasper <djasper at google.com>
>> > wrote:
>> >>
>> >> As for the naming: I would leave it as is. Two reasons:
>> >> - We would also need to change the other one to
>> >> ignoreParenAndImpCasts()
>> >> and I dislike 5+ word method names. 5+ word matcher names will not make
>> >> indentation of matcher-definitions any easier.
>> >> - We deviate from what people familiar with the AST are used to. I
>> >> would
>> >> prefer the matcher language being reasonably close to the corresponding
>> >> AST
>> >> entities/functions.
>> >
>> >
>> > Okay. For now, I'll leave the matcher as ignoreParenImplicitCasts() to
>> > parallel the AST; it can be changed later .
>> >
>> >>
>> >>
>> >>
>> >> +/// void* c = reinterpret_cast<char*>(&c);
>> >> +/// char d = char(0);
>> >> +/// The matcher
>> >> +/// variable(hasInitializer(ignoringParenCasts(integerLiteral())))
>> >> +/// would match the declarations for a, b, c, and d.
>> >>
>> >> I strongly doubt it would match c's declaration.
>> >
>> >
>> > I strongly agree. This was supposed to be "void* c =
>> > reinterpret_cast<char*>(0);"
>> >
>> >> /// class URL { URL(string); };
>> >> /// URL url = "a string";
>> >> -AST_MATCHER_P(ImplicitCastExpr, hasSourceExpression,
>> >> +AST_MATCHER_P(CastExpr, hasSourceExpression,
>> >>
>> >> I think Manuel was quicker with this one. Should go away with a sync.
>> >>
>> >
>> > It did.
>> >
>> >>
>> >> return InnerMatcher.matches(NodeType, Finder, Builder);
>> >> }
>> >>
>> >> +
>> >> /// \brief Matches implicit casts whose destination type matches a
>> >> given
>> >> /// matcher.
>> >>
>> >> Please revert the newline...
>> >
>> >
>> > Done.
>> >
>> >>
>> >>
>> >> +TEST(CastExpression, MatchesSimpleCases) {
>> >> + EXPECT_TRUE(matches("char *p = reinterpret_cast<char *>(&p);",
>> >> + expression(castExpression())));
>> >>
>> >> I think just "castExpression()" instead of
>> >> "expression(castExpression())"
>> >> should be fine.
>> >
>> >
>> > Actually, without the expression() wrapper, I get a compiler error:
>> > (slightly abridged)
>> >
>> > ../tools/clang/unittests/ASTMatchers/ASTMatchersTest.h:57:10: error:
>> > call to
>> > member function 'addMatcher' is ambiguous
>> > Finder.addMatcher(AMatcher, new VerifyMatch(0, &Found));
>> > note: candidate function
>> > void addMatcher(const DeclarationMatcher &NodeMatch,
>> > note: candidate function
>> > void addMatcher(const TypeMatcher &NodeMatch,
>> > note: candidate function
>> > void addMatcher(const StatementMatcher &NodeMatch,
>> >
>> > I'm not sure what the correct fix is, but adding expression() makes the
>> > compiler happy. This test was modeled after the existing ReinterpretCast
>> > tests, which do the same thing :)
>> > I wonder that it's related to the compiler error I received when trying
>> > to
>> > use id("name", allOf(...)), which I worked around by explicitly
>> > specifying
>> > the template parameter to id().
>> >
>> >>
>> >>
>> >> + EXPECT_TRUE(matches("int x = 0; const int y = x;",
>> >> +
>> >> variable(hasInitializer(implicitCast(expression())))));
>> >>
>> >> I think writing "implicitCast()" should lead to the same behavior as
>> >> "implicitCast(expression())".
>> >>
>> >
>> > Yep. Simplified as suggested.
>> >
>> >>
>> >> +TEST(IgnoringParenCasts, MatchesOnlyWithParenCasts) {
>> >> + EXPECT_TRUE(notMatches("int x = (0);",
>> >> +
>> >> variable(hasInitializer(integerLiteral(equals(0))))));
>> >>
>> >> This EXPECT_TRUE(...) is duplicated (4 lines below).
>> >
>> >
>> > Fixed.
>> >
>> >>
>> >>
>> >> In general, it feels like the tests could be more focused. There are a
>> >> lot
>> >> of tests (which is good), but I find it very hard to tell, which cases
>> >> are
>> >> actually well-tested and what might still be untested. I think what
>> >> might
>> >> help is more documentation:
>> >>
>> >> - Write for each test (not necessarily for each EXPECT_TRUE, but at
>> >> least
>> >> for each function), a one-sentence comment describing the purpose of
>> >> this
>> >> test. Example:
>> >>
>> >> TEST(ImplicitCast, DoesNotMatchIncorrectly) {
>> >> // This tests ensures that implicitCast() only matches when a cast is
>> >> present and
>> >> // ignores explicit casts.
>> >
>> >
>> > I tried to do this for the non-obvious tests (e.g. "MatchesSimpleCase"
>> > probably doesn't need such a comment). I think that one of the problems
>> > was
>> > the MatchesOnlyWithXXXCasts tests that were intended to check what
>> > happens
>> > with the addition of ignoringXXXCasts. The negative examples don't seem
>> > truly necessary, so I removed them and renamed the tests. The
>> > MatchesWithAndWithoutXXXCasts had a similar problem.
>> >
>> >>
>> >>
>> >> When I wrote this comment, I actually noticed that there is no test
>> >> case
>> >> to test for paren-casts. Might be worthwhile to add one ("int x =
>> >> (0);").
>> >>
>> >
>> > Done. And a few more missing test cases were added (e.g. C-style casts
>> > in
>> > ignoringImplicitCasts).
>> >
>> >
>> >>
>> >> - For the non-obvious cases, explain the AST-structure that is actually
>> >> being created. Example:
>> >>
>> >
>> > I added comments notomg all the relevant implicit casts being created in
>> > these test cases.
>> >
>> >>
>> >> TEST(IgnoringParenAndImpCasts, MatchesOnlyWithParenImpCasts) {
>> >> // The following tests create an implicit const-cast.
>> >> EXPECT_TRUE(notMatches("int x = 0; const int y = x;",
>> >>
>> >> variable(hasInitializer(declarationReference()))));
>> >> EXPECT_TRUE(matches("int x = 0; const int y = x;",
>> >>
>> >> variable(hasInitializer(ignoringParenAndImplicitCasts(
>> >> declarationReference())))));
>> >>
>> >> I know this is very hand-waiving, but I hope you get the idea.
>> >
>> >
>> > I think it was clear enough to follow :)
>> > How does this update look?
>> >
>> >>
>> >>
>> >> Cheers,
>> >> Daniel
>> >>
>> >>
>> >>
>> >>
>> >> On Tue, Jul 24, 2012 at 11:10 PM, Manuel Klimek <klimek at google.com>
>> >> wrote:
>> >>>
>> >>> On Tue, Jul 24, 2012 at 9:03 PM, Sam Panzer <panzer at google.com> wrote:
>> >>> >
>> >>> >
>> >>> >
>> >>> > On Tue, Jul 24, 2012 at 1:12 AM, Daniel Jasper <djasper at google.com>
>> >>> > wrote:
>> >>> >>
>> >>> >> +/// \brief Matches expressions that match InnerMatcher after any
>> >>> >> implicit
>> >>> >> casts
>> >>> >> +/// are stripped off. Parentheses and explicit casts are not
>> >>> >> discarded.
>> >>> >> +/// Given
>> >>> >>
>> >>> >> Two line breaks after "stripped off.". As far as I know, the
>> >>> >> brief-comment
>> >>> >> should always be a single, reasonably short sentence. Same for the
>> >>> >> next
>> >>> >> comment.
>> >>> >
>> >>> >
>> >>> > Done.
>> >>> >
>> >>> >>
>> >>> >>
>> >>> >> +/// \brief Matches expressions that match InnerMatcher after
>> >>> >> parentheses
>> >>> >> +/// are stripped off. Implicit and explicit casts are not
>> >>> >> discarded.
>> >>> >>
>> >>> >> The second sentence IMO is the opposite of true :-).
>> >>> >
>> >>> >
>> >>> > Fixed.
>> >>> >
>> >>> >>
>> >>> >>
>> >>> >> +/// Given
>> >>> >> +/// int a = (0);
>> >>> >> +/// char b = 0;
>> >>> >> +/// char c = (0);
>> >>> >> +/// char d = (char)0;
>> >>> >> +/// The matcher
>> >>> >> +///
>> >>> >> variable(hasInitializer(ignoringParenCasts(integerLiteral(0))))
>> >>> >> +/// would match the declarations for a, b, and c, but not d.
>> >>> >>
>> >>> >> From the description of Expr::IngoreParenCasts(), I would assume it
>> >>> >> does
>> >>> >> match the declaration of d as well. That seems to be the main
>> >>> >> difference
>> >>> >> between IgnoreParenCasts() and IgnoreParenImpCasts(). Also there
>> >>> >> seems
>> >>> >> to be
>> >>> >> a test supporting me ;-) (TEST(IgnoringParenCasts,
>> >>> >> MatchesOnlyWithParenCasts)).
>> >>> >
>> >>> >
>> >>> > This comment was old and, yes, incorrect :) It's been fixed.
>> >>> >
>> >>> >>
>> >>> >>
>> >>> >> +const internal::VariadicDynCastAllOfMatcher<
>> >>> >> + Stmt,
>> >>> >> + MaterializeTemporaryExpr> materializeTemporaryExpression;
>> >>> >>
>> >>> >> Did you add this to this patch intentionally? Does not seem to be
>> >>> >> related
>> >>> >> and there are no tests for it.
>> >>> >>
>> >>> >
>> >>> > No, this belongs in a separate patch. I didn't correctly separate
>> >>> > out
>> >>> > new
>> >>> > matcher features this time...
>> >>> >
>> >>> >
>> >>> >>
>> >>> >> +TEST(IgnoringImplicitCasts, DoesNotMatchIncorrectly) {
>> >>> >> + EXPECT_TRUE(notMatches("int x; const int y = x;",
>> >>> >> +
>> >>> >> variable(hasInitializer(ignoringImplicitCasts(
>> >>> >> + integerLiteral(equals(0)))))));
>> >>> >>
>> >>> >> This (and some others like it) are somewhat useless tests with
>> >>> >> regard
>> >>> >> to
>> >>> >> the cast expressions. There is no way that the matcher could ever
>> >>> >> match
>> >>> >> something in the given source code, independent of whether the
>> >>> >> cast-matcher
>> >>> >> does or does not do what it is supposed to do. The only thing you
>> >>> >> might
>> >>> >> catch is that it always returns true. If you want to test that,
>> >>> >> make
>> >>> >> the
>> >>> >> inner part: unless(anything()).
>> >>> >
>> >>> >
>> >>> > The first tests in each DoesNotMatchIncorrectly pair was intended to
>> >>> > check
>> >>> > the case where the inner matcher doesn't match, and the second test
>> >>> > was
>> >>> > to
>> >>> > check the chase where the inner matcher does match, but the cast
>> >>> > expression
>> >>> > doesn't. As you point out, these are better expressed as
>> >>> > unless(anything()).
>> >>> >
>> >>> >>
>> >>> >>
>> >>> >> + EXPECT_TRUE(notMatches(
>> >>> >> + "int x = 0; float *y = (reinterpret_cast<float *>(&x));",
>> >>> >> +
>> >>> >>
>> >>> >>
>> >>> >> variable(hasInitializer(ignoringParenCasts(declarationReference())))));
>> >>> >>
>> >>> >> I think, this might be confusing. If I understand it right, the
>> >>> >> test
>> >>> >> fails, because the declarationReference is child of a unaryOperator
>> >>> >> ("&").
>> >>> >> The IgnoreParenCasts would happily remove the reinterpret_cast.
>> >>> >
>> >>> >
>> >>> > I had this backwards - I thought IgnoreParenCasts was a subset of
>> >>> > IgnoreParenImpCasts, but it's really the other way around. Would it
>> >>> > be
>> >>> > a
>> >>> > good idea to rename ignoringParenCasts to ignoringParensAndCasts?
>> >>>
>> >>> Spontaneous +1. This has caused me quite a few resets of my brain and
>> >>> wasted 2 minutes :)
>> >>>
>> >>> >
>> >>> >>
>> >>> >>
>> >>> >> + EXPECT_TRUE(notMatches("int x = 0; float *y =
>> >>> >> reinterpret_cast<float*>(&x);",
>> >>> >> +
>> >>> >> variable(hasInitializer(ignoringParenAndImplicitCasts(
>> >>> >> + declarationReference())))));
>> >>> >>
>> >>> >> Same as above. These tests are not only confusing, but also not
>> >>> >> essentially helpful. This test would still pass (i.e. the operator
>> >>> >> not
>> >>> >> match) if ignoreParenAndImplicitCasts() would suddenly "eat" the
>> >>> >> reinterpret_cast as the matcher does not match on the unaryOperator
>> >>> >> inside
>> >>> >> it.
>> >>> >>
>> >>> >
>> >>> > I cleaned up the tests. Are these better?
>> >>> >
>> >>> >>
>> >>> >>
>> >>> >>
>> >>> >> On Tue, Jul 24, 2012 at 12:26 AM, Sam Panzer <panzer at google.com>
>> >>> >> wrote:
>> >>> >>>
>> >>> >>>
>> >>> >>> This thread forked from a previous group of patches, now with a
>> >>> >>> new
>> >>> >>> and
>> >>> >>> improved version of additional cast-related matchers.
>> >>> >>>
>> >>> >>> On Sun, Jul 15, 2012 at 1:44 PM, Daniel Jasper
>> >>> >>> <djasper at google.com>
>> >>> >>> wrote:
>> >>> >>>>
>> >>> >>>> Hi Sam,
>> >>> >>>>
>> >>> >>>>
>> >>> >>>> ignore-casts.patch:
>> >>> >>>> First of, I think these methods are going to be really helpful!
>> >>> >>>>
>> >>> >>>> I am not sure about the naming. I am not a native speaker, but:
>> >>> >>>> variable(hasInitializer(ignoreImplicitCasts(expression())))
>> >>> >>>> feels a bit "rough". How about:
>> >>> >>>> variable(hasInitializer(ignoringImplicitCasts(expression())))
>> >>> >>>> Still does not feel ideal. Any thoughts?
>> >>> >>>>
>> >>> >>>
>> >>> >>> I went with "ignoringImplicitCasts" as suggested though
>> >>> >>> "withoutImplicitCasts" could also work.
>> >>> >>>
>> >>> >>>>
>> >>> >>>> The implementation definitely needs more elaborate comments with
>> >>> >>>> examples of what the matchers are supposed to match.
>> >>> >>>
>> >>> >>>
>> >>> >>> I added much more in-depth comments, complete with examples that
>> >>> >>> differentiate the different kinds of casts involved here. It
>> >>> >>> almost
>> >>> >>> seems
>> >>> >>> that I'm duplicating some AST documentation though, since these
>> >>> >>> matchers
>> >>> >>> just forward a method call to the inner matcher. In this case,
>> >>> >>> ignoringParenCasts does exactly what Expr::IgnoreParenCasts() is
>> >>> >>> supposed to
>> >>> >>> do.
>> >>> >>> This made me wonder if it would be reasonable to create a macro
>> >>> >>> that
>> >>> >>> makes writing this kind of trivial matcher easier. I imagine
>> >>> >>> something like
>> >>> >>> this:
>> >>> >>>
>> >>> >>> AST_MATCHER_METHOD(Expr, ignoringParenAndImplicitCasts,
>> >>> >>> internal::Matcher<Expr>, IgnoreParenImpCasts);
>> >>> >>> which expands to the same result as
>> >>> >>> AST_MATCHER_P(Expr, ignoringParenAndImplicitCasts,
>> >>> >>> internal::Matcher<Expr>, InnerMatcher) {
>> >>> >>> return InnerMatcher.matches(*Node.IgnoreParenImpCasts(),
>> >>> >>> Finder,
>> >>> >>> Builder);
>> >>> >>> }
>> >>> >>>
>> >>> >>> Though this would only be useful if there are a good number of
>> >>> >>> accessors
>> >>> >>> to add, though I found around 15 existing matchers that just
>> >>> >>> forwarded one
>> >>> >>> method call and about 15 more matchers test the result of a method
>> >>> >>> call for
>> >>> >>> NULL before passing it on to an inner matcher. I expect that
>> >>> >>> including a
>> >>> >>> full predicate would be overkill, though.
>> >>> >>>
>> >>> >>>>
>> >>> >>>> And the tests should also include these examples with both
>> >>> >>>> positive
>> >>> >>>> and
>> >>> >>>> negative cases.
>> >>> >>>
>> >>> >>>
>> >>> >>> Tests added!
>> >>> >>>
>> >>> >>>>
>> >>> >>>> Also, I think you misunderstand what the ignoreParenCasts is
>> >>> >>>> mainly
>> >>> >>>> supposed to do. A ParenExpr is added if you put parenthesis
>> >>> >>>> around
>> >>> >>>> an
>> >>> >>>> expression. Thus, the ignoreParenCasts-method mainly handles
>> >>> >>>> cases
>> >>> >>>> like "int
>> >>> >>>> i = (0);". It also strips of casts and thus the CStyleCastExpr in
>> >>> >>>> "const int
>> >>> >>>> y = (const int) x;". This needs documentation and tests.
>> >>> >>>>
>> >>> >>>
>> >>> >>> You're right: I didn't realize that ignoreParenCasts meant
>> >>> >>> stripping
>> >>> >>> parentheses. This has been fixed :)
>> >>> >>>
>> >>> >>> I also added a CastExpression matcher and changed the
>> >>> >>> hasSourceExpression
>> >>> >>> matcher to require any CastExpression, rather than an
>> >>> >>> ImplicitCastExpression, as I wanted to use it to match an explicit
>> >>> >>> cast to
>> >>> >>> any integer type (since some for loop authors cast to int rather
>> >>> >>> than
>> >>> >>> using
>> >>> >>> unsigned loop indices).
>> >>> >>>
>> >>> >>>
>> >>> >>>>
>> >>> >>>> On Fri, Jul 13, 2012 at 8:34 PM, Sam Panzer <panzer at google.com>
>> >>> >>>> wrote:
>> >>> >>>>>
>> >>> >>>>> <div>Attached are three more small matcher patches. One fixes
>> >>> >>>>> another
>> >>> >>>>> rename typo (AnyOf --> anyOf) that was similar to the
>> >>> >>>>> previous
>> >>> >>>>> allOf patch. The second patch adds more inspection for
>> >>> >>>>> declarationStatement matchers, making it easier to look at
>> >>> >>>>> single
>> >>> >>>>> declarations directly. The third patch adds expression matchers
>> >>> >>>>> which
>> >>> >>>>> call IgnoreXXXCasts() before applying their
>> >>> >>>>> sub-matchers.</div><div><br></div>For future reference, should I
>> >>> >>>>> continue splitting up these patches for
>> >>> >>>>> review?<div><br></div><div>-Sam</div>
>> >>> >>>>
>> >>> >>>>
>> >>> >>>
>> >>> >>
>> >>> >
>> >>> >
>> >>> > _______________________________________________
>> >>> > cfe-commits mailing list
>> >>> > cfe-commits at cs.uiuc.edu
>> >>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >>> >
>> >>
>> >>
>> >
>
>
More information about the cfe-commits
mailing list