On Wed, Jul 25, 2012 at 7:14 PM, Sam Panzer <[email protected]> wrote:
> > > 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? > I think that depends on whether you want to become a continuous clang-contributer. For now, I am happy to submit these patches. You should ask Chandler what he thinks. > > >> >> >>> >>> >>>> >>>> >>>> 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
