[cfe-commits] [PATCH] x3 More matcher patches
Sam Panzer
panzer at google.com
Thu Aug 16 10:22:17 PDT 2012
Thanks, commited r162027.
On Thu, Aug 16, 2012 at 10:05 AM, Manuel Klimek <klimek at google.com> wrote:
>
> On Thu, Aug 16, 2012 at 7:03 PM, Sam Panzer <panzer at google.com> wrote:
> > Should I go ahead and commit this one too?
>
> Nobody committed that yet? Oh, of course! :)
>
> >
> >
> > On Mon, Jul 30, 2012 at 4:18 AM, Manuel Klimek <klimek at google.com>
> wrote:
> >>
> >>
> >> 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
> >> >> >>
> >> >> >>
> >> >> >
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120816/01877126/attachment.html>
More information about the cfe-commits
mailing list