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

[Sam Panzer]

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
> >>
> >>
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120723/931d3616/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: decl-matcher.patch
Type: application/octet-stream
Size: 4747 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120723/931d3616/attachment.obj>