[cfe-commits] [PATCH]: Matchers for ignoring paren/implicit casts
Manuel Klimek
klimek at google.com
Thu Jul 26 01:53:27 PDT 2012
+/// \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...
+/// \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;
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 ;)
+ 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.
+ pointsTo(TypeMatcher(anything())))))));
This is unfortunate. We should add a type() matcher.
+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.
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