[cfe-commits] [PATCH] x3 More matcher patches

Wed Jul 25 10:14:21 PDT 2012

On Wed, Jul 25, 2012 at 6:41 AM, Daniel Jasper <djasper at google.com> wrote:

>
>
>
> On Tue, Jul 24, 2012 at 7:16 PM, Sam Panzer <panzer at google.com> wrote:
>
>>
>> Suggestions applied, and the new patch is attached.
>>
>> On Tue, Jul 24, 2012 at 6:23 AM, Daniel Jasper <djasper at google.com>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 <klimek at google.com>wrote:
>>>
>>>> lgtm
>>>>
>>>> On Mon, Jul 23, 2012 at 8:30 PM, Sam Panzer <panzer at google.com> wrote:
>>>> > Latest version attached!
>>>> >
>>>> > On Tue, Jul 17, 2012 at 2:07 AM, Manuel Klimek <klimek at google.com>
>>>> wrote:
>>>> >>
>>>> >> On Tue, Jul 17, 2012 at 12:47 AM, Sam Panzer <panzer at google.com>
>>>> 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 <panzer at google.com>
>>>> 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 <klimek at google.com
>>>> >
>>>> >> >> wrote:
>>>> >> >>>
>>>> >> >>>
>>>> >> >>> On Mon, Jul 16, 2012 at 6:22 PM, David Blaikie <
>>>> dblaikie at gmail.com>
>>>> >> >>> wrote:
>>>> >> >>> > On Mon, Jul 16, 2012 at 12:03 AM, Manuel Klimek <
>>>> klimek at google.com>
>>>> >> >>> > 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 <
>>>> panzer at google.com>
>>>> >> >>> >> 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
>>>> >> >>> >>> cfe-commits at cs.uiuc.edu
>>>> >> >>> >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>> >> >>> >>>
>>>> >> >>> >> _______________________________________________
>>>> >> >>> >> cfe-commits mailing list
>>>> >> >>> >> cfe-commits at cs.uiuc.edu
>>>> >> >>> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>> >> >>
>>>> >> >>
>>>> >> >
>>>> >
>>>> >
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120725/02ef9972/attachment.html>