[cfe-commits] [PATCH] x3 More matcher patches

Sam Panzer panzer at google.com
Tue Jul 24 10:16:00 PDT 2012 

Suggestions applied, and the new patch is attached.

On Tue, Jul 24, 2012 at 6:23 AM, Daniel Jasper <djasper at google.com> wrote:

> +/// \brief Matches the Decl of a DeclStmt which has a single declaration.
> +/// Given
>
> I think you need an empty line between the \brief and the rest of the
> comment in order for doxygen to understand it correctly. (Here and in the
> other comments).
>

Thanks for the heads up!


>
> +/// \brief Matches the n'th declaration of a declaration statement.
> +/// Note that this does not work for global declarations due to the AST
>
> nit: "." at the end and separate with an empty comment line.
>

This was actually due to me leaving off the end of the sentence. It's now
included.


>
> I would prefer: "Note that this does not work for global declarations as
> they are not represented by declarations statements in the AST."
>
> +///  declarationStatement(containsDeclaration(
> +///       0, variable(hasInitializer(anything()))))
> +///   matches only 'int d = 2, e;', and
> +///  declarationStatement(containsDeclaration(1, variable()))
>
> nit^2: The declarationStatements seem to be indented one more than in your
> other comments.
>
>
Fixed.


> +  EXPECT_TRUE(matches("void f() {int i,j; int k;}",
> +                      declarationStatement(declCountIs(2))));
>
> I think the "int k;" is unnecessary and makes the test (very slightly)
> worse. It would also pass if declCountIs(2) would for some very strange
> reason only match on a single declaration. For this test it is somewhat
> pointless, but in general, the examples should be as narrow as possible for
> the positive tests and as wide as possible for the negative tests (there,
> it might actually make sense to include "int a, b, c, d;" to clarify/ensure
> that declCountIs() does not match on declaration statements with at least N
> declarations). Again, no strong need to change this here, just as general
> advise.
>

I think I added this test to check that the matcher didn't only work on
code containing exactly one DeclStmt, though it doesn't really make sense,
as you point out.

I also renamed the HasDecl test to ContainsDeclaration to reflect the
matcher's name change.


> Other than these nits, the patch looks good! Thank you!
>

Does anything else need fixing?


>
>
> On Tue, Jul 24, 2012 at 2:58 PM, Manuel Klimek <klimek at google.com> wrote:
>
>> lgtm
>>
>> On Mon, Jul 23, 2012 at 8:30 PM, Sam Panzer <panzer at google.com> wrote:
>> > Latest version attached!
>> >
>> > On Tue, Jul 17, 2012 at 2:07 AM, Manuel Klimek <klimek at google.com>
>> wrote:
>> >>
>> >> On Tue, Jul 17, 2012 at 12:47 AM, Sam Panzer <panzer at google.com>
>> wrote:
>> >> > I also noticed that a hasDeclaration matcher which serves a different
>> >> > purpose. I think the new hasDecl matcher needs a new name...
>> >>
>> >> Good point. Any ideas for how to differentiate "hasDeclaration" in
>> >> terms of "something that declares what's used here" vs.
>> >> "hasDeclaration" in terms of "aggregates a declaration that matches"?
>> >>
>> >
>> > How about "containsDeclaration" for the latter case? Since it's
>> intended for
>> > use in a DeclStmt, it makes sense to talk about the declarations
>> contained
>> > within the statement - and I think it would be difficult to find a
>> better
>> > name for the original "hasDeclaration."
>> >
>> >
>> >>
>> >> >
>> >> >
>> >> > On Mon, Jul 16, 2012 at 3:45 PM, Sam Panzer <panzer at google.com>
>> wrote:
>> >> >>
>> >> >> Here's a new version of the DeclStmt patch. Changes include:
>> >> >>  - Fixed comments by declCountIs and hasSingleDecl
>> >> >>  - Added hasDecl in the spirit of hasArgument
>> >> >>  - Changed the loop to std::distance (std::advance in hasDecl)
>> >> >>  - Added a few more test cases.
>> >> >>
>> >> >> And to explain the for loop in the test case for hasSingleDecl, I
>> >> >> discovered that Clang explodes some DeclStmts with multiple
>> >> >> declarations
>> >> >> such as these:
>> >> >>   int a, b;  // toplevel declarations
>> >> >> According to the AST dump, Clang treats this line as two separate
>> >> >> DeclStmts, rather than one DeclStmt with two Decls. This also
>> happens
>> >> >> to
>> >> >> declarations inside namespaces, and I'm not really sure where else.
>> >> >> Maybe
>> >> >> someone else has a better idea how to describe when the AST doesn't
>> >> >> reflect
>> >> >> the source the same way?
>> >> >>
>> >> >> The other patch will be sent on a fork of the previous discussion.
>> >> >> Any new comments?
>> >> >>
>> >> >> On Mon, Jul 16, 2012 at 9:39 AM, Manuel Klimek <klimek at google.com>
>> >> >> wrote:
>> >> >>>
>> >> >>>
>> >> >>> On Mon, Jul 16, 2012 at 6:22 PM, David Blaikie <dblaikie at gmail.com
>> >
>> >> >>> wrote:
>> >> >>> > On Mon, Jul 16, 2012 at 12:03 AM, Manuel Klimek <
>> klimek at google.com>
>> >> >>> > wrote:
>> >> >>> >> +  // We could use Node.decl_begin() - Node.decl_end(), but that
>> >> >>> >> relies on
>> >> >>> >> +  // decl_iterator just being a Decl**.
>> >> >>> >> +  unsigned DeclCount = 0;
>> >> >>> >> +  for (DeclStmt::const_decl_iterator I = Node.decl_begin(),
>> >> >>> >> +       E = Node.decl_end(); I != E; ++I)
>> >> >>> >> +    ++DeclCount;
>> >> >>> >>
>> >> >>> >> (after chatting with Chandler about this on irc):
>> >> >>> >> I'd use Node.decl_end() - Node.decl_begin(). If it ever becomes
>> a
>> >> >>> >> non-const-time operation, the iterator will not implement the
>> >> >>> >> interface and break compilation, so we'll notice.
>> >> >>> >
>> >> >>> > But do we need to notice? If the original algorithm written here
>> is
>> >> >>> > linear it seems like constant time size is not a requirement.
>> >> >>> >
>> >> >>> > If that's the case, then std::distance just DTRT - constant time
>> for
>> >> >>>
>> >> >>> I personally am fine with arguing for std::distance. My point is
>> not
>> >> >>> to write the loop :)
>> >> >>>
>> >> >>> > random access iterators, linear for others. (alternatively,
>> provide
>> >> >>> > Node::decl_size that does the same thing - but I can understand
>> the
>> >> >>> > concern of providing a (possibly in the future) non-constant-time
>> >> >>> > size, though at that point you could remove size & go back &
>> examine
>> >> >>> > each client to see which ones care about that)
>> >> >>> >
>> >> >>> >>
>> >> >>> >> Regardless of that, I think your comment is wrong in 2 ways:
>> first,
>> >> >>> >> there's a typo :) Second, that the iterator happens to come
>> down do
>> >> >>> >> being a pointer has nothing to do with its contract. It either
>> >> >>> >> provides random access or not.
>> >> >>> >>
>> >> >>> >> +/// \brief Matches expressions that match InnerMatcher after
>> >> >>> >> implicit
>> >> >>> >> casts are
>> >> >>> >> +/// stripped off.
>> >> >>> >> +AST_MATCHER_P(Expr, ignoreImplicitCasts,
>> >> >>> >> +              internal::Matcher<Expr>, InnerMatcher) {
>> >> >>> >> +  return InnerMatcher.matches(*Node.IgnoreImpCasts(), Finder,
>> >> >>> >> Builder);
>> >> >>> >> +}
>> >> >>> >>
>> >> >>> >> I think we should implement the equivalent based on
>> >> >>> >> ignoreParenImpCast
>> >> >>> >> first, as that's what I've seen us needing much more often (we
>> can
>> >> >>> >> implement this one, too, of course ;)
>> >> >>> >>
>> >> >>> >> Cheers,
>> >> >>> >> /Manuel
>> >> >>> >>
>> >> >>> >> 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
>> >> >>> >>>
>> >> >>> >> _______________________________________________
>> >> >>> >> cfe-commits mailing list
>> >> >>> >> cfe-commits at cs.uiuc.edu
>> >> >>> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >> >>
>> >> >>
>> >> >
>> >
>> >
>> _______________________________________________
>> 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/20120724/939b2e9f/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: decl-matcher.patch
Type: application/octet-stream
Size: 4981 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120724/939b2e9f/attachment.obj>

More information about the cfe-commits mailing list