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. > Not really. > > > >> + 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. > What I meant was that the positive case should only contain: "int i, j;" whereas the negative case should have "int i; int j, k; int a, b, c, d;" to rule out any other possible matching reason: - int i; tests that it does not match on single-decl declarationStatements. - int j, k; tests that declCountIs() does not mean declCountIsAtMost() - int a, b, c, d; tests that declCountIs() does not mean declCountIsAtLeast() As I said, this is probably over-engineering for this case, but I wanted to get the point across and it does not really hurt. > 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? > No, the patch looks good. Do you have commit access by now or should I submit it? > > >> >> >> 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
