[cfe-commits] [PATCH]: Matchers for ignoring paren/implicit casts
Daniel Jasper
djasper at google.com
Wed Jul 25 07:11:28 PDT 2012
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.
+/// 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.
/// 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.
return InnerMatcher.matches(NodeType, Finder, Builder);
}
+
/// \brief Matches implicit casts whose destination type matches a given
/// matcher.
Please revert the newline...
+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.
+ 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())".
+TEST(IgnoringParenCasts, MatchesOnlyWithParenCasts) {
+ EXPECT_TRUE(notMatches("int x = (0);",
+
variable(hasInitializer(integerLiteral(equals(0))))));
This EXPECT_TRUE(...) is duplicated (4 lines below).
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.
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);").
- For the non-obvious cases, explain the AST-structure that is actually
being created. Example:
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.
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/eab5c4c9/attachment.html>
More information about the cfe-commits
mailing list