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

[Manuel Klimek]

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
>