Thanks, commited r162027.<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Aug 16, 2012 at 10:05 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im"><br>
On Thu, Aug 16, 2012 at 7:03 PM, Sam Panzer <<a href="mailto:panzer@google.com">panzer@google.com</a>> wrote:<br>
> Should I go ahead and commit this one too?<br>
<br>
</div>Nobody committed that yet? Oh, of course! :)<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
><br>
> On Mon, Jul 30, 2012 at 4:18 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>><br>
>><br>
>> lgtm<br>
>><br>
>> On Thu, Jul 26, 2012 at 8:01 PM, Sam Panzer <<a href="mailto:panzer@google.com">panzer@google.com</a>> wrote:<br>
>> ><br>
>> > Here's the next version :)<br>
>> ><br>
>> > On Thu, Jul 26, 2012 at 2:00 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >><br>
>> >> +AST_POLYMORPHIC_MATCHER_P2(<br>
>> >> + containsDeclaration, unsigned, N, internal::Matcher<Decl>,<br>
>> >> InnerMatcher) {<br>
>> >> + TOOLING_COMPILE_ASSERT((llvm::is_base_of<DeclStmt,<br>
>> >> NodeType>::value),<br>
>> >> + has_decl_requires_decl_stmt);<br>
>> >><br>
>> >> This doesn't really make sense to me: why have a polymorphic return<br>
>> >> type and then check that it's in a single type path?<br>
>> >> Saying AST_MATCHER_P2(DeclStmt, ...) should have the same effect, as<br>
>> >> Matcher<DeclStmt> is-a Matcher<Type-derived-from-decl-stmt>.<br>
>> >><br>
>> ><br>
>> > Fixed. For good measure, I also deleted the extra spaces that Daniel<br>
>> > pointed<br>
>> > out.<br>
>> ><br>
>> > -Sam<br>
>> ><br>
>> >><br>
>> >> Apart from that LGTM<br>
>> >><br>
>> >> Cheers,<br>
>> >> /Manuel<br>
>> >><br>
>> >> On Tue, Jul 24, 2012 at 7:16 PM, Sam Panzer <<a href="mailto:panzer@google.com">panzer@google.com</a>> wrote:<br>
>> >> ><br>
>> >> > Suggestions applied, and the new patch is attached.<br>
>> >> ><br>
>> >> > On Tue, Jul 24, 2012 at 6:23 AM, Daniel Jasper <<a href="mailto:djasper@google.com">djasper@google.com</a>><br>
>> >> > wrote:<br>
>> >> >><br>
>> >> >> +/// \brief Matches the Decl of a DeclStmt which has a single<br>
>> >> >> declaration.<br>
>> >> >> +/// Given<br>
>> >> >><br>
>> >> >> I think you need an empty line between the \brief and the rest of<br>
>> >> >> the<br>
>> >> >> comment in order for doxygen to understand it correctly. (Here and<br>
>> >> >> in<br>
>> >> >> the<br>
>> >> >> other comments).<br>
>> >> ><br>
>> >> ><br>
>> >> > Thanks for the heads up!<br>
>> >> ><br>
>> >> >><br>
>> >> >><br>
>> >> >> +/// \brief Matches the n'th declaration of a declaration statement.<br>
>> >> >> +/// Note that this does not work for global declarations due to the<br>
>> >> >> AST<br>
>> >> >><br>
>> >> >> nit: "." at the end and separate with an empty comment line.<br>
>> >> ><br>
>> >> ><br>
>> >> > This was actually due to me leaving off the end of the sentence. It's<br>
>> >> > now<br>
>> >> > included.<br>
>> >> ><br>
>> >> >><br>
>> >> >><br>
>> >> >> I would prefer: "Note that this does not work for global<br>
>> >> >> declarations<br>
>> >> >> as<br>
>> >> >> they are not represented by declarations statements in the AST."<br>
>> >> >><br>
>> >> >> +/// declarationStatement(containsDeclaration(<br>
>> >> >> +/// 0, variable(hasInitializer(anything()))))<br>
>> >> >> +/// matches only 'int d = 2, e;', and<br>
>> >> >> +/// declarationStatement(containsDeclaration(1, variable()))<br>
>> >> >><br>
>> >> >> nit^2: The declarationStatements seem to be indented one more than<br>
>> >> >> in<br>
>> >> >> your<br>
>> >> >> other comments.<br>
>> >> >><br>
>> >> ><br>
>> >> > Fixed.<br>
>> >> ><br>
>> >> >><br>
>> >> >> + EXPECT_TRUE(matches("void f() {int i,j; int k;}",<br>
>> >> >> + declarationStatement(declCountIs(2))));<br>
>> >> >><br>
>> >> >> I think the "int k;" is unnecessary and makes the test (very<br>
>> >> >> slightly)<br>
>> >> >> worse. It would also pass if declCountIs(2) would for some very<br>
>> >> >> strange<br>
>> >> >> reason only match on a single declaration. For this test it is<br>
>> >> >> somewhat<br>
>> >> >> pointless, but in general, the examples should be as narrow as<br>
>> >> >> possible<br>
>> >> >> for<br>
>> >> >> the positive tests and as wide as possible for the negative tests<br>
>> >> >> (there, it<br>
>> >> >> might actually make sense to include "int a, b, c, d;" to<br>
>> >> >> clarify/ensure<br>
>> >> >> that declCountIs() does not match on declaration statements with at<br>
>> >> >> least N<br>
>> >> >> declarations). Again, no strong need to change this here, just as<br>
>> >> >> general<br>
>> >> >> advise.<br>
>> >> ><br>
>> >> ><br>
>> >> > I think I added this test to check that the matcher didn't only work<br>
>> >> > on<br>
>> >> > code<br>
>> >> > containing exactly one DeclStmt, though it doesn't really make sense,<br>
>> >> > as<br>
>> >> > you<br>
>> >> > point out.<br>
>> >> ><br>
>> >> > I also renamed the HasDecl test to ContainsDeclaration to reflect the<br>
>> >> > matcher's name change.<br>
>> >> ><br>
>> >> >><br>
>> >> >> Other than these nits, the patch looks good! Thank you!<br>
>> >> ><br>
>> >> ><br>
>> >> > Does anything else need fixing?<br>
>> >> ><br>
>> >> >><br>
>> >> >><br>
>> >> >><br>
>> >> >> On Tue, Jul 24, 2012 at 2:58 PM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>><br>
>> >> >> wrote:<br>
>> >> >>><br>
>> >> >>> lgtm<br>
>> >> >>><br>
>> >> >>> On Mon, Jul 23, 2012 at 8:30 PM, Sam Panzer <<a href="mailto:panzer@google.com">panzer@google.com</a>><br>
>> >> >>> wrote:<br>
>> >> >>> > Latest version attached!<br>
>> >> >>> ><br>
>> >> >>> > On Tue, Jul 17, 2012 at 2:07 AM, Manuel Klimek<br>
>> >> >>> > <<a href="mailto:klimek@google.com">klimek@google.com</a>><br>
>> >> >>> > wrote:<br>
>> >> >>> >><br>
>> >> >>> >> On Tue, Jul 17, 2012 at 12:47 AM, Sam Panzer <<a href="mailto:panzer@google.com">panzer@google.com</a>><br>
>> >> >>> >> wrote:<br>
>> >> >>> >> > I also noticed that a hasDeclaration matcher which serves a<br>
>> >> >>> >> > different<br>
>> >> >>> >> > purpose. I think the new hasDecl matcher needs a new name...<br>
>> >> >>> >><br>
>> >> >>> >> Good point. Any ideas for how to differentiate "hasDeclaration"<br>
>> >> >>> >> in<br>
>> >> >>> >> terms of "something that declares what's used here" vs.<br>
>> >> >>> >> "hasDeclaration" in terms of "aggregates a declaration that<br>
>> >> >>> >> matches"?<br>
>> >> >>> >><br>
>> >> >>> ><br>
>> >> >>> > How about "containsDeclaration" for the latter case? Since it's<br>
>> >> >>> > intended for<br>
>> >> >>> > use in a DeclStmt, it makes sense to talk about the declarations<br>
>> >> >>> > contained<br>
>> >> >>> > within the statement - and I think it would be difficult to find<br>
>> >> >>> > a<br>
>> >> >>> > better<br>
>> >> >>> > name for the original "hasDeclaration."<br>
>> >> >>> ><br>
>> >> >>> ><br>
>> >> >>> >><br>
>> >> >>> >> ><br>
>> >> >>> >> ><br>
>> >> >>> >> > On Mon, Jul 16, 2012 at 3:45 PM, Sam Panzer<br>
>> >> >>> >> > <<a href="mailto:panzer@google.com">panzer@google.com</a>><br>
>> >> >>> >> > wrote:<br>
>> >> >>> >> >><br>
>> >> >>> >> >> Here's a new version of the DeclStmt patch. Changes include:<br>
>> >> >>> >> >> - Fixed comments by declCountIs and hasSingleDecl<br>
>> >> >>> >> >> - Added hasDecl in the spirit of hasArgument<br>
>> >> >>> >> >> - Changed the loop to std::distance (std::advance in<br>
>> >> >>> >> >> hasDecl)<br>
>> >> >>> >> >> - Added a few more test cases.<br>
>> >> >>> >> >><br>
>> >> >>> >> >> And to explain the for loop in the test case for<br>
>> >> >>> >> >> hasSingleDecl,<br>
>> >> >>> >> >> I<br>
>> >> >>> >> >> discovered that Clang explodes some DeclStmts with multiple<br>
>> >> >>> >> >> declarations<br>
>> >> >>> >> >> such as these:<br>
>> >> >>> >> >> int a, b; // toplevel declarations<br>
>> >> >>> >> >> According to the AST dump, Clang treats this line as two<br>
>> >> >>> >> >> separate<br>
>> >> >>> >> >> DeclStmts, rather than one DeclStmt with two Decls. This also<br>
>> >> >>> >> >> happens<br>
>> >> >>> >> >> to<br>
>> >> >>> >> >> declarations inside namespaces, and I'm not really sure where<br>
>> >> >>> >> >> else.<br>
>> >> >>> >> >> Maybe<br>
>> >> >>> >> >> someone else has a better idea how to describe when the AST<br>
>> >> >>> >> >> doesn't<br>
>> >> >>> >> >> reflect<br>
>> >> >>> >> >> the source the same way?<br>
>> >> >>> >> >><br>
>> >> >>> >> >> The other patch will be sent on a fork of the previous<br>
>> >> >>> >> >> discussion.<br>
>> >> >>> >> >> Any new comments?<br>
>> >> >>> >> >><br>
>> >> >>> >> >> On Mon, Jul 16, 2012 at 9:39 AM, Manuel Klimek<br>
>> >> >>> >> >> <<a href="mailto:klimek@google.com">klimek@google.com</a>><br>
>> >> >>> >> >> wrote:<br>
>> >> >>> >> >>><br>
>> >> >>> >> >>><br>
>> >> >>> >> >>> On Mon, Jul 16, 2012 at 6:22 PM, David Blaikie<br>
>> >> >>> >> >>> <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> >>> >> >>> wrote:<br>
>> >> >>> >> >>> > On Mon, Jul 16, 2012 at 12:03 AM, Manuel Klimek<br>
>> >> >>> >> >>> > <<a href="mailto:klimek@google.com">klimek@google.com</a>><br>
>> >> >>> >> >>> > wrote:<br>
>> >> >>> >> >>> >> + // We could use Node.decl_begin() - Node.decl_end(),<br>
>> >> >>> >> >>> >> but<br>
>> >> >>> >> >>> >> that<br>
>> >> >>> >> >>> >> relies on<br>
>> >> >>> >> >>> >> + // decl_iterator just being a Decl**.<br>
>> >> >>> >> >>> >> + unsigned DeclCount = 0;<br>
>> >> >>> >> >>> >> + for (DeclStmt::const_decl_iterator I =<br>
>> >> >>> >> >>> >> Node.decl_begin(),<br>
>> >> >>> >> >>> >> + E = Node.decl_end(); I != E; ++I)<br>
>> >> >>> >> >>> >> + ++DeclCount;<br>
>> >> >>> >> >>> >><br>
>> >> >>> >> >>> >> (after chatting with Chandler about this on irc):<br>
>> >> >>> >> >>> >> I'd use Node.decl_end() - Node.decl_begin(). If it ever<br>
>> >> >>> >> >>> >> becomes<br>
>> >> >>> >> >>> >> a<br>
>> >> >>> >> >>> >> non-const-time operation, the iterator will not implement<br>
>> >> >>> >> >>> >> the<br>
>> >> >>> >> >>> >> interface and break compilation, so we'll notice.<br>
>> >> >>> >> >>> ><br>
>> >> >>> >> >>> > But do we need to notice? If the original algorithm<br>
>> >> >>> >> >>> > written<br>
>> >> >>> >> >>> > here<br>
>> >> >>> >> >>> > is<br>
>> >> >>> >> >>> > linear it seems like constant time size is not a<br>
>> >> >>> >> >>> > requirement.<br>
>> >> >>> >> >>> ><br>
>> >> >>> >> >>> > If that's the case, then std::distance just DTRT -<br>
>> >> >>> >> >>> > constant<br>
>> >> >>> >> >>> > time<br>
>> >> >>> >> >>> > for<br>
>> >> >>> >> >>><br>
>> >> >>> >> >>> I personally am fine with arguing for std::distance. My<br>
>> >> >>> >> >>> point<br>
>> >> >>> >> >>> is<br>
>> >> >>> >> >>> not<br>
>> >> >>> >> >>> to write the loop :)<br>
>> >> >>> >> >>><br>
>> >> >>> >> >>> > random access iterators, linear for others.<br>
>> >> >>> >> >>> > (alternatively,<br>
>> >> >>> >> >>> > provide<br>
>> >> >>> >> >>> > Node::decl_size that does the same thing - but I can<br>
>> >> >>> >> >>> > understand<br>
>> >> >>> >> >>> > the<br>
>> >> >>> >> >>> > concern of providing a (possibly in the future)<br>
>> >> >>> >> >>> > non-constant-time<br>
>> >> >>> >> >>> > size, though at that point you could remove size & go back<br>
>> >> >>> >> >>> > &<br>
>> >> >>> >> >>> > examine<br>
>> >> >>> >> >>> > each client to see which ones care about that)<br>
>> >> >>> >> >>> ><br>
>> >> >>> >> >>> >><br>
>> >> >>> >> >>> >> Regardless of that, I think your comment is wrong in 2<br>
>> >> >>> >> >>> >> ways:<br>
>> >> >>> >> >>> >> first,<br>
>> >> >>> >> >>> >> there's a typo :) Second, that the iterator happens to<br>
>> >> >>> >> >>> >> come<br>
>> >> >>> >> >>> >> down do<br>
>> >> >>> >> >>> >> being a pointer has nothing to do with its contract. It<br>
>> >> >>> >> >>> >> either<br>
>> >> >>> >> >>> >> provides random access or not.<br>
>> >> >>> >> >>> >><br>
>> >> >>> >> >>> >> +/// \brief Matches expressions that match InnerMatcher<br>
>> >> >>> >> >>> >> after<br>
>> >> >>> >> >>> >> implicit<br>
>> >> >>> >> >>> >> casts are<br>
>> >> >>> >> >>> >> +/// stripped off.<br>
>> >> >>> >> >>> >> +AST_MATCHER_P(Expr, ignoreImplicitCasts,<br>
>> >> >>> >> >>> >> + internal::Matcher<Expr>, InnerMatcher) {<br>
>> >> >>> >> >>> >> + return InnerMatcher.matches(*Node.IgnoreImpCasts(),<br>
>> >> >>> >> >>> >> Finder,<br>
>> >> >>> >> >>> >> Builder);<br>
>> >> >>> >> >>> >> +}<br>
>> >> >>> >> >>> >><br>
>> >> >>> >> >>> >> I think we should implement the equivalent based on<br>
>> >> >>> >> >>> >> ignoreParenImpCast<br>
>> >> >>> >> >>> >> first, as that's what I've seen us needing much more<br>
>> >> >>> >> >>> >> often<br>
>> >> >>> >> >>> >> (we<br>
>> >> >>> >> >>> >> can<br>
>> >> >>> >> >>> >> implement this one, too, of course ;)<br>
>> >> >>> >> >>> >><br>
>> >> >>> >> >>> >> Cheers,<br>
>> >> >>> >> >>> >> /Manuel<br>
>> >> >>> >> >>> >><br>
>> >> >>> >> >>> >> On Fri, Jul 13, 2012 at 8:34 PM, Sam Panzer<br>
>> >> >>> >> >>> >> <<a href="mailto:panzer@google.com">panzer@google.com</a>><br>
>> >> >>> >> >>> >> wrote:<br>
>> >> >>> >> >>> >>> <div>Attached are three more small matcher patches. One<br>
>> >> >>> >> >>> >>> fixes<br>
>> >> >>> >> >>> >>> another<br>
>> >> >>> >> >>> >>> rename typo (AnyOf --> anyOf) that was similar to the<br>
>> >> >>> >> >>> >>> previous<br>
>> >> >>> >> >>> >>> allOf patch. The second patch adds more inspection for<br>
>> >> >>> >> >>> >>> declarationStatement matchers, making it easier to look<br>
>> >> >>> >> >>> >>> at<br>
>> >> >>> >> >>> >>> single<br>
>> >> >>> >> >>> >>> declarations directly. The third patch adds expression<br>
>> >> >>> >> >>> >>> matchers<br>
>> >> >>> >> >>> >>> which<br>
>> >> >>> >> >>> >>> call IgnoreXXXCasts() before applying their<br>
>> >> >>> >> >>> >>> sub-matchers.</div><div><br></div>For future reference,<br>
>> >> >>> >> >>> >>> should<br>
>> >> >>> >> >>> >>> I<br>
>> >> >>> >> >>> >>> continue splitting up these patches for<br>
>> >> >>> >> >>> >>> review?<div><br></div><div>-Sam</div><br>
>> >> >>> >> >>> >>><br>
>> >> >>> >> >>> >>> _______________________________________________<br>
>> >> >>> >> >>> >>> cfe-commits mailing list<br>
>> >> >>> >> >>> >>> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> >> >>> >> >>> >>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>> >> >>> >> >>> >>><br>
>> >> >>> >> >>> >> _______________________________________________<br>
>> >> >>> >> >>> >> cfe-commits mailing list<br>
>> >> >>> >> >>> >> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> >> >>> >> >>> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>> >> >>> >> >><br>
>> >> >>> >> >><br>
>> >> >>> >> ><br>
>> >> >>> ><br>
>> >> >>> ><br>
>> >> >>> _______________________________________________<br>
>> >> >>> cfe-commits mailing list<br>
>> >> >>> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> >> >>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>> >> >><br>
>> >> >><br>
>> >> ><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div>