recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

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/7d3a9040/attachment-0001.html>