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

Daniel Jasper djasper at google.com
Tue Jul 24 06:23:39 PDT 2012


+/// \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).

+/// \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.

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.

+  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.

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


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/ab26743e/attachment.html>


More information about the cfe-commits mailing list