<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jul 24, 2012 at 7:16 PM, Sam Panzer <span dir="ltr"><<a href="mailto:panzer@google.com" target="_blank" class="cremed">panzer@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br><div class="gmail_extra">Suggestions applied, and the new patch is attached.<br><br><div class="gmail_quote"><div class="im">
On Tue, Jul 24, 2012 at 6:23 AM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank" class="cremed">djasper@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>+/// \brief Matches the Decl of a DeclStmt which has a single declaration.</div><div>+/// Given</div><div><br></div>
<div>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).</div></blockquote><div><br></div></div><div>Thanks for the heads up! </div>
<div class="im">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div><div>+/// \brief Matches the n'th declaration of a declaration statement.</div><div>+/// Note that this does not work for global declarations due to the AST</div></div><div><br></div><div>nit: "." at the end and separate with an empty comment line.</div>
</blockquote><div><br></div></div><div>This was actually due to me leaving off the end of the sentence. It's now included.</div><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div>I would prefer: "Note that this does not work for global declarations as they are not represented by declarations statements in the AST."</div><div><br></div><div><div>+/// declarationStatement(containsDeclaration(</div>
<div>+/// 0, variable(hasInitializer(anything()))))</div><div>+/// matches only 'int d = 2, e;', and</div><div>+/// declarationStatement(containsDeclaration(1, variable()))</div></div><div><br></div><div>
nit^2: The declarationStatements seem to be indented one more than in your other comments.</div><div><br></div></blockquote><div><br></div></div><div>Fixed.</div></div></div></blockquote><div><br></div><div>Not really.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div> </div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_extra"><div class="gmail_quote"><div class="im"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div></div><div><div>+ EXPECT_TRUE(matches("void f() {int i,j; int k;}",</div><div>+ declarationStatement(declCountIs(2))));</div>
</div><div><br></div><div>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.</div>
</blockquote><div><br></div></div><div>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. </div></div>
</div></blockquote><div><br></div><div>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:</div>
<div>- int i; tests that it does not match on single-decl declarationStatements.</div><div>- int j, k; tests that declCountIs() does not mean declCountIsAtMost()</div><div>- int a, b, c, d; tests that declCountIs() does not mean declCountIsAtLeast()</div>
<div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_extra"><div class="gmail_quote"><div>I also renamed the HasDecl test to ContainsDeclaration to reflect the matcher's name change. </div><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br></div><div>Other than these nits, the patch looks good! Thank you!</div></blockquote><div><br></div></div><div>Does anything else need fixing?</div></div></div></blockquote><div><br></div><div>No, the patch looks good. Do you have commit access by now or should I submit it?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jul 24, 2012 at 2:58 PM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank" class="cremed">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">lgtm<br>
<div><div><br>
On Mon, Jul 23, 2012 at 8:30 PM, Sam Panzer <<a href="mailto:panzer@google.com" target="_blank" class="cremed">panzer@google.com</a>> wrote:<br>
> Latest version attached!<br>
><br>
> On Tue, Jul 17, 2012 at 2:07 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank" class="cremed">klimek@google.com</a>> wrote:<br>
>><br>
>> On Tue, Jul 17, 2012 at 12:47 AM, Sam Panzer <<a href="mailto:panzer@google.com" target="_blank" class="cremed">panzer@google.com</a>> wrote:<br>
>> > I also noticed that a hasDeclaration matcher which serves a different<br>
>> > purpose. I think the new hasDecl matcher needs a new name...<br>
>><br>
>> Good point. Any ideas for how to differentiate "hasDeclaration" in<br>
>> terms of "something that declares what's used here" vs.<br>
>> "hasDeclaration" in terms of "aggregates a declaration that matches"?<br>
>><br>
><br>
> How about "containsDeclaration" for the latter case? Since it's intended for<br>
> use in a DeclStmt, it makes sense to talk about the declarations contained<br>
> within the statement - and I think it would be difficult to find a better<br>
> name for the original "hasDeclaration."<br>
><br>
><br>
>><br>
>> ><br>
>> ><br>
>> > On Mon, Jul 16, 2012 at 3:45 PM, Sam Panzer <<a href="mailto:panzer@google.com" target="_blank" class="cremed">panzer@google.com</a>> 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 hasDecl)<br>
>> >> - Added a few more test cases.<br>
>> >><br>
>> >> And to explain the for loop in the test case for hasSingleDecl, 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 separate<br>
>> >> DeclStmts, rather than one DeclStmt with two Decls. This also happens<br>
>> >> to<br>
>> >> declarations inside namespaces, and I'm not really sure where else.<br>
>> >> Maybe<br>
>> >> someone else has a better idea how to describe when the AST doesn't<br>
>> >> reflect<br>
>> >> the source the same way?<br>
>> >><br>
>> >> The other patch will be sent on a fork of the previous discussion.<br>
>> >> Any new comments?<br>
>> >><br>
>> >> On Mon, Jul 16, 2012 at 9:39 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank" class="cremed">klimek@google.com</a>><br>
>> >> wrote:<br>
>> >>><br>
>> >>><br>
>> >>> On Mon, Jul 16, 2012 at 6:22 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank" class="cremed">dblaikie@gmail.com</a>><br>
>> >>> wrote:<br>
>> >>> > On Mon, Jul 16, 2012 at 12:03 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank" class="cremed">klimek@google.com</a>><br>
>> >>> > wrote:<br>
>> >>> >> + // We could use Node.decl_begin() - Node.decl_end(), but that<br>
>> >>> >> relies on<br>
>> >>> >> + // decl_iterator just being a Decl**.<br>
>> >>> >> + unsigned DeclCount = 0;<br>
>> >>> >> + for (DeclStmt::const_decl_iterator I = 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 becomes a<br>
>> >>> >> non-const-time operation, the iterator will not implement the<br>
>> >>> >> interface and break compilation, so we'll notice.<br>
>> >>> ><br>
>> >>> > But do we need to notice? If the original algorithm written here is<br>
>> >>> > linear it seems like constant time size is not a requirement.<br>
>> >>> ><br>
>> >>> > If that's the case, then std::distance just DTRT - constant time for<br>
>> >>><br>
>> >>> I personally am fine with arguing for std::distance. My point is not<br>
>> >>> to write the loop :)<br>
>> >>><br>
>> >>> > random access iterators, linear for others. (alternatively, provide<br>
>> >>> > Node::decl_size that does the same thing - but I can understand the<br>
>> >>> > concern of providing a (possibly in the future) non-constant-time<br>
>> >>> > size, though at that point you could remove size & go back & 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 ways: first,<br>
>> >>> >> there's a typo :) Second, that the iterator happens to come down do<br>
>> >>> >> being a pointer has nothing to do with its contract. It either<br>
>> >>> >> provides random access or not.<br>
>> >>> >><br>
>> >>> >> +/// \brief Matches expressions that match InnerMatcher 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(), 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 often (we 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 <<a href="mailto:panzer@google.com" target="_blank" class="cremed">panzer@google.com</a>><br>
>> >>> >> wrote:<br>
>> >>> >>> <div>Attached are three more small matcher patches. One fixes<br>
>> >>> >>> another<br>
>> >>> >>> rename typo (AnyOf --> anyOf) that was similar to the previous<br>
>> >>> >>> allOf patch. The second patch adds more inspection for<br>
>> >>> >>> declarationStatement matchers, making it easier to look at single<br>
>> >>> >>> declarations directly. The third patch adds expression matchers<br>
>> >>> >>> which<br>
>> >>> >>> call IgnoreXXXCasts() before applying their<br>
>> >>> >>> sub-matchers.</div><div><br></div>For future reference, should 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" target="_blank" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
>> >>> >>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">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" target="_blank" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
>> >>> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">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" target="_blank" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div></div></div><br></div>
</blockquote></div><br></div>