[cfe-commits] [PATCH] x3 More matcher patches
Manuel Klimek
klimek at google.com
Thu Jul 26 02:00:33 PDT 2012
+AST_POLYMORPHIC_MATCHER_P2(
+ containsDeclaration, unsigned, N, internal::Matcher<Decl>, InnerMatcher) {
+ TOOLING_COMPILE_ASSERT((llvm::is_base_of<DeclStmt, NodeType>::value),
+ has_decl_requires_decl_stmt);
This doesn't really make sense to me: why have a polymorphic return
type and then check that it's in a single type path?
Saying AST_MATCHER_P2(DeclStmt, ...) should have the same effect, as
Matcher<DeclStmt> is-a Matcher<Type-derived-from-decl-stmt>.
Apart from that LGTM
Cheers,
/Manuel
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.
>
>>
>> + 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.
>
> 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?
>
>>
>>
>>
>> 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
>>
>>
>
More information about the cfe-commits
mailing list