recordDecl() AST matcher

[Richard Smith via cfe-commits]

I don't think CXXRecordDecl is an anachronism, so much as an implementation
detail; it makes sense to use a smaller class when in C mode, as we don't
need most of the features and complexity that CXXRecordDecl brings with it.
But... as a user of clang matchers, I don't think I'd want to care about
the difference, and it'd be more convenient if I could nest (say) a
hasMethod matcher within a recordDecl matcher, since it's completely
obvious what that should mean. If I have a matcher that says:

  recordDecl(or(hasMethod(...), hasField(...)))

I would expect that to work in both C and C++ (and the only way it could
match in C would be on a record with the specified field, since the
hasMethod matcher would always fail).

On Fri, Sep 11, 2015 at 6:30 AM, Manuel Klimek <klimek at google.com> wrote:

> Richard! We need an informed opinion :D
>
> On Fri, Sep 11, 2015 at 3:07 PM Aaron Ballman <aaron at aaronballman.com>
> wrote:
>
>> Ping?
>>
>> On Tue, Sep 8, 2015 at 9:26 AM, Manuel Klimek <klimek at google.com> wrote:
>> > On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman <aaron at aaronballman.com>
>> wrote:
>> >>
>> >> On Tue, Sep 8, 2015 at 9:18 AM, Manuel Klimek <klimek at google.com>
>> wrote:
>> >> > On Tue, Sep 8, 2015 at 2:23 PM Aaron Ballman <aaron at aaronballman.com
>> >
>> >> > wrote:
>> >> >>
>> >> >> On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek <klimek at google.com>
>> >> >> wrote:
>> >> >> > Yea, we had that discussion a few times, and I can never remember
>> why
>> >> >> > we
>> >> >> > ended up in the state we're in.
>> >> >> > We definitely had a time where we switched to just using the exact
>> >> >> > same
>> >> >> > name
>> >> >> > as the node's class name for the matchers.
>> >> >> > I *think* we didn't do it for cxxRecordDecl, because Richard said
>> >> >> > that's
>> >> >> > a
>> >> >> > relic we should get rid of anyway, but I'm not sure.
>> >> >>
>> >> >> FWIW, I think the state we're in is the worst of all worlds. It's
>> not
>> >> >> intuitive that recordDecl() doesn't match a struct in C mode, and as
>> >> >> it stands, there is no way to match a struct or union declaration
>> in C
>> >> >> at all.
>> >> >
>> >> >
>> >> > Agreed. Best intentions. Worst possible outcome. That's software
>> >> > development
>> >> > :)
>> >> >
>> >> >> >
>> >> >> > On Fri, Sep 4, 2015 at 8:32 PM Aaron Ballman <
>> aaron at aaronballman.com>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> It turns out that the recordDecl() AST matcher doesn't match
>> >> >> >> RecordDecl objects; instead, it matches CXXRecordDecl objects.
>> This
>> >> >> >> is... unfortunate... as it makes writing AST matchers more
>> >> >> >> complicated
>> >> >> >> because of having to translate between
>> recordDecl()/CXXRecordDecl.
>> >> >> >> It
>> >> >> >> also makes it impossible to match a struct or union declaration
>> in C
>> >> >> >> or ObjC. However, given how prevalent recordDecl()'s use is in
>> the
>> >> >> >> wild (I'm guessing), changing it at this point would be a Bad
>> Thing.
>> >> >> >>
>> >> >> >> For people trying to write AST matchers for languages like C or
>> >> >> >> ObjC,
>> >> >> >> I would like to propose adding:
>> >> >> >>
>> >> >> >> structDecl()
>> >> >> >> unionDecl()
>> >> >> >> tagDecl()
>> >> >> >>
>> >> >> >> These will match nicely with the existing enumDecl() AST matcher.
>> >> >> >>
>> >> >> >> Additionally, I would like to add cxxRecordDecl() to match
>> >> >> >> CXXRecordDecl objects. While it duplicates the functionality
>> exposed
>> >> >> >> by recordDecl(), it more clearly matches the intention of which
>> AST
>> >> >> >> node it corresponds to.
>> >> >> >>
>> >> >> >> Finally, I would like to undocument recordDecl() and change our
>> >> >> >> existing documentation and AST matcher uses to use
>> >> >> >> cxxRecordDecl/structDecl() instead. Maybe someday we can
>> deprecate
>> >> >> >> recordDecl() more officially.
>> >> >> >>
>> >> >> >> I'm open to other ideas if there are better ways to move
>> forward. If
>> >> >> >> you think changing the meaning of recordDecl() is acceptable, I
>> can
>> >> >> >> also go that route (though I would still propose adding
>> unionDecl()
>> >> >> >> and cxxRecordDecl() in that case).
>> >> >> >
>> >> >> >
>> >> >> > I think changing recordDecl is acceptable. I believe very few
>> tools
>> >> >> > will
>> >> >> > actually start doing wrong things because of it. I'd like more
>> >> >> > opinions
>> >> >> > first, though :)
>> >> >>
>> >> >> I was giving this more thought over the long weekend, and I think
>> you
>> >> >> may be right. I think changing recordDecl() to mean RecordDecl will
>> >> >> fix more code than it breaks, so long as we take a holistic approach
>> >> >> to the change and see which narrowing and traversal matchers we need
>> >> >> to fix up at the same time. When I tried to think of AST matchers
>> that
>> >> >> mean CXXRecordDecl but *not* RecordDecl, they were horribly
>> contrived
>> >> >> because you usually are matching on additional selection criteria
>> that
>> >> >> is specific to C++ (such as hasMethod() or isDerivedFrom()) which
>> >> >> would cause the match to continue to fail, as expected. Code that
>> uses
>> >> >> recordDecl() to mean RecordDecl will suddenly start to match in more
>> >> >> cases, but that's likely to be a bug fix more than a breaking
>> change.
>> >> >> To the best of my understanding, the only breaking cases would be
>> >> >> where you wrote recordDecl(), meant CXXRecordDecl, had no further
>> >> >> narrowing or traversal matchers, and were compiling in C mode; with
>> >> >> the result being additional unexpected matches.
>> >> >
>> >> >
>> >> > Ah, there's one thing that can break: the compile can break:
>> >> > recordDecl(hasMethod(...)) will *not* compile (it'll work in the
>> dynamic
>> >> > matchers and fail as you suggest, but the in-C++ DSL does more static
>> >> > type
>> >> > checking).
>> >> > I don't think that's super bad though.
>> >> >
>> >> >>
>> >> >> So perhaps it would make sense to:
>> >> >>
>> >> >> 1) Make recordDecl() mean RecordDecl
>> >> >> 2) Do a comprehensive review of matchers that take a CXXRecordDecl
>> and
>> >> >> see if they should instead take a RecordDecl
>> >> >> 3) Add unionDecl() as a node matcher (or should we add isUnion() and
>> >> >> isStruct() as narrowing matchers?)
>> >> >> 4) Add tagDecl() as a node matcher, but not add cxxRecordDecl()
>> >> >
>> >> >
>> >> > Why not add cxxRecordDecl()? I think we need it if we want narrowing
>> >> > matchers on CXXRecordDecl?
>> >>
>> >> If Richard thinks CXXRecordDecl is an anachronism, I figured we didn't
>> >> want to expose it. Instead, we could make hasMethod (et al) accept a
>> >> RecordDecl and do the type checking for the caller. Then
>> >> recordDecl(hasMethod(...)) continues to compile and work, and when
>> >> hasMethod is given a RecordDecl instead of a CXXRecordDecl, it simply
>> >> matches nothing. But you bring up a good point about the C++ DSL being
>> >> a problem still, I hadn't considered that.
>> >
>> >
>> > First I want Richard to confirm that. I have a very bad memory, so I
>> might
>> > as well misremember :)
>> >
>> >>
>> >>
>> >> ~Aaron
>> >>
>> >> >
>> >> >>
>> >> >>
>> >> >> ~Aaron
>> >> >>
>> >> >> >
>> >> >> >>
>> >> >> >>
>> >> >> >> Thanks!
>> >> >> >>
>> >> >> >> ~Aaron
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150911/b74a4626/attachment-0001.html>