recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman <aaron at aaronballman.com>
wrote:

> On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > 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).
>
> Okay, so then it sounds like we want recordDecl to *mean* RecordDecl,
> but we want the traversal and narrowing matchers that currently take a
> CXXRecordDecl to instead take a RecordDecl and handle the CXX part
> transparently? This means we would not need to add a cxxRecordDecl()
> matcher, but could still access CXX-only functionality (like access
> control, base classes, etc) through recordDecl()?
>

I'm against that proposal. I think we have tried to make the matchers more
"user friendly" in the past, and all those attempts have failed miserably;
in the end, users will do ast-dump to see what they want to match, and then
be confused when the matchers do follow the AST 99% of the time, but try to
be smart 1% of the time.
I think given that we want to keep CXXRecordDecl, the right solution is to
have a cxxRecordDecl() matcher.

Richard: if CXXRecordDecl was really an implementation detail, it would be
hidden behind a RecordDecl class, as an implementation detail. The reasons
why we don't want it to be an implementation detail in the code
(performance, data structure size) don't matter - in the end, it's in the
AST API.


>
> ~Aaron
>
> >
> > 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/20150912/83837f8a/attachment.html>