On Wed, Jul 25, 2012 at 6:41 AM, Daniel Jasper <[email protected]> wrote:
> > > > 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. > One more space needs to be deleted on the two lines. Apparently my text editor doesn't indent code inside comments correctly. > > >> >> > >> >>> + 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. > And thanks for taking the time to make sure I understood :) > > >> 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? > No, I don't even have an svn account. Would it be a good idea to get one? > > >> >> >>> >>> >>> 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
