recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

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.

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

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()

~Aaron

>
>>
>>
>> Thanks!
>>
>> ~Aaron