[cfe-commits] [PATCH]: Matchers for ignoring paren/implicit casts

Sam Panzer panzer at google.com
Wed Jul 25 14:01:53 PDT 2012


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
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120725/94299817/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cast-matchers-update.patch
Type: application/octet-stream
Size: 14602 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120725/94299817/attachment.obj>


More information about the cfe-commits mailing list