lgtm
On Thu, Jul 26, 2012 at 8:01 PM, Sam Panzer <[email protected]> wrote: > > Here's the next version :) > > On Thu, Jul 26, 2012 at 2:00 AM, Manuel Klimek <[email protected]> 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 <[email protected]> wrote: >> > >> > Suggestions applied, and the new patch is attached. >> > >> > On Tue, Jul 24, 2012 at 6:23 AM, Daniel Jasper <[email protected]> >> > 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 <[email protected]> >> >> wrote: >> >>> >> >>> 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 >> >> >> >> >> > > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
