[cfe-commits] [PATCH]: Matchers for ignoring paren/implicit casts
Sam Panzer
panzer at google.com
Thu Jul 26 14:15:48 PDT 2012
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.
>
> + 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?
> + 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 :)
>
> +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
> >>> >
> >>
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120726/d14e9873/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cast-matchers-update-2.patch
Type: application/octet-stream
Size: 15494 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120726/d14e9873/attachment.obj>
More information about the cfe-commits
mailing list