Thanks, commited r162027.
On Thu, Aug 16, 2012 at 10:05 AM, Manuel Klimek <[email protected]> wrote: > > On Thu, Aug 16, 2012 at 7:03 PM, Sam Panzer <[email protected]> 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 <[email protected]> > wrote: > >> > >> > >> 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
