recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

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