recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

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?


>
> ~Aaron
>
> >
> >>
> >>
> >> Thanks!
> >>
> >> ~Aaron
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150908/6fb6f238/attachment.html>