lgtm
On Mon, Jul 23, 2012 at 8:30 PM, Sam Panzer <[email protected]> wrote: > Latest version attached! > > On Tue, Jul 17, 2012 at 2:07 AM, Manuel Klimek <[email protected]> wrote: >> >> On Tue, Jul 17, 2012 at 12:47 AM, Sam Panzer <[email protected]> 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 <[email protected]> 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 <[email protected]> >> >> wrote: >> >>> >> >>> >> >>> On Mon, Jul 16, 2012 at 6:22 PM, David Blaikie <[email protected]> >> >>> wrote: >> >>> > On Mon, Jul 16, 2012 at 12:03 AM, Manuel Klimek <[email protected]> >> >>> > 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 <[email protected]> >> >>> >> 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 >> >>> >>> [email protected] >> >>> >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >>> >>> >> >>> >> _______________________________________________ >> >>> >> cfe-commits mailing list >> >>> >> [email protected] >> >>> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >> >> >> >> > > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
