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

Daniel Jasper djasper at google.com
Wed Jul 25 10:20:15 PDT 2012 

On Wed, Jul 25, 2012 at 7:14 PM, Sam Panzer <panzer at google.com> wrote:

>
>
> 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?
>

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 <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/1d81df4b/attachment.html>

More information about the cfe-commits mailing list