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

[Daniel Jasper]

+/// \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.

+/// \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 :-).

+/// 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)).

+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.

+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()).

+  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.

+  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.



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>
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120724/88273476/attachment.html>