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

[Sam Panzer]

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