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

Manuel Klimek klimek at google.com
Mon Jul 30 04:18:20 PDT 2012 

lgtm

On Thu, Jul 26, 2012 at 8:01 PM, Sam Panzer <panzer at google.com> wrote:
>
> Here's the next version :)
>
> On Thu, Jul 26, 2012 at 2:00 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>>
>> +AST_POLYMORPHIC_MATCHER_P2(
>> +    containsDeclaration, unsigned, N, internal::Matcher<Decl>,
>> InnerMatcher) {
>> +  TOOLING_COMPILE_ASSERT((llvm::is_base_of<DeclStmt, NodeType>::value),
>> +                         has_decl_requires_decl_stmt);
>>
>> This doesn't really make sense to me: why have a polymorphic return
>> type and then check that it's in a single type path?
>> Saying AST_MATCHER_P2(DeclStmt, ...) should have the same effect, as
>> Matcher<DeclStmt> is-a Matcher<Type-derived-from-decl-stmt>.
>>
>
> Fixed. For good measure, I also deleted the extra spaces that Daniel pointed
> out.
>
> -Sam
>
>>
>> Apart from that LGTM
>>
>> Cheers,
>> /Manuel
>>
>> On Tue, Jul 24, 2012 at 7:16 PM, Sam Panzer <panzer at google.com> wrote:
>> >
>> > 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
>> >>
>> >>
>> >
>
>

More information about the cfe-commits mailing list