recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

On Mon, Sep 14, 2015 at 2:45 PM, Daniel Jasper <djasper at google.com> wrote:
> By this point, I see that change might be profitable overall. However, lets
> completely map this out. Changing just cxxRecordDecl() can actually increase
> confusion in other areas. Right now, not a single AST matcher has the cxx
> prefix (although a total of 28 stand for the corresponding CXX.. AST node).
> This is consistent and people knowing this will never try to write
> cxxConstructExpr(). As soon as people have used cxxRecordDecl(), the chance
> of them trying cxxConstructExpr() increases. You have spent a long time
> figuring out that recordDecl means cxxRecordDecl(), which is one datapoint,
> but I am not aware of anyone else having this specific issue. And we could
> make this less bad with better documentation, I think.
>
> So, for me, the questions are:
> 1) Do we want/need this change?

We definitely need *a* change because there currently is no way to
match a C struct or union when compiling in C mode. I discovered this
because I was trying to write a new checker for clang-tidy that
focuses on C code and it would fail to match when compiling in C mode.
Whether we decide to go with cxxRecordDecl vs recordDecl vs structDecl
(etc) is less important to me than the ability to write clang-tidy
checks for C code.

> 2) Do we want to be consistent and change the other 27 matchers as well?

I'm on the fence about this for all the reasons you point out.

> A fundamental question is whether we want AST matchers to match AST nodes
> 1:1 or whether they should be an abstraction from some implementation
> details of the AST.

I absolutely agree that this is a fundamental question. I think a
higher priority fundamental question that goes along with it is: are
we okay with breaking a lot of user code (are these meant to be stable
APIs like the LLVM C APIs)? If we want these APIs to be stable, that
changes the answer of what kind of mapping we can have.

> And this is not an easy question to answer. There are
> many places where we don't follow a strict 1:1 mapping. Mostly node
> matchers, but also in traversal matchers, e.g. isDerivedFrom().
>
> Personally, I'd really hate to have the cxx Prefix everywhere, but that's
> just my personal opinion. I would even prefer matchers like record() and
> method(), but I think somebody convinced me that that would be a very bad
> idea ;-).

My personal opinion is that (1) breaking code is fine, but we should
avoid doing it without very clear benefit, and (2) the mapping between
AST node identifiers and AST matcher identifiers needs to be
incredibly obvious, but perhaps not slavishly 1:1. If we instead
decide we want a 1:1 mapping, then I think we need to start seriously
considering auto-generating the AST node (and type) matchers from
tablegen so that the AST nodes *cannot* get out of sync with the AST
matchers, otherwise we'll be right back here again in a few years when
we modify the name of an AST node.

My definition of "incredibly obvious" is: if the AST has a prefixed
and unprefixed version, or two different prefixes, we should mimic
that directly with the matchers. Otherwise, existing AST matchers
without prefix shenanigans can remain as they are, and new AST
matchers should prefix as-required. If we decide we're okay breaking
code, then I don't see a problem with changing ctorInitializer() into
cxxCtorInitializer() when C adds constructors. ;-)

I should be clear, I'm not opposed to just having a 1:1 mapping. I'm
just not certain the benefits of being strict about that outweigh the
costs to broken code. cxxCtorInitializer will break someone's code,
but I don't think it adds any clarity to their code, so I don't see
the benefit of forcing the change.

~Aaron

>
>
> On Mon, Sep 14, 2015 at 8:08 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Mon, Sep 14, 2015 at 1:37 PM, Manuel Klimek <klimek at google.com> wrote:
>> > On Mon, Sep 14, 2015 at 10:30 AM Daniel Jasper <djasper at google.com>
>> > wrote:
>> >>
>> >> On Mon, Sep 14, 2015 at 7:24 PM, Manuel Klimek <klimek at google.com>
>> >> wrote:
>> >>>
>> >>> On Mon, Sep 14, 2015 at 10:21 AM Daniel Jasper <djasper at google.com>
>> >>> wrote:
>> >>>>
>> >>>> So, back in the day when we implemented the matchers, we decided on
>> >>>> actually wanting to remove all the CXX... AST nodes (there are more
>> >>>> of
>> >>>> them).
>> >>>
>> >>>
>> >>> Note that Richard has paddled back on this and now says the CXX... AST
>> >>> nodes are the right thing.
>> >>>
>> >>>>
>> >>>> I don't know how this would work as recordDecl already exists. But
>> >>>> I'd
>> >>>> be somewhat hesitant to introduce a cxxRecordDecl matcher if there is
>> >>>> still
>> >>>> a chance that we want to move away from the CXX prefix.
>> >>>
>> >>>
>> >>> See above.
>> >>
>> >>
>> >> So do we change all the others at the same time, e.g. create a
>> >> cxxConstructExpr() matcher? There it make less sense as there is no
>> >> ConstructExpr, just CXXConstructExpr.
>> >>
>> >>>> Also note that AST matchers are used massively in the wild by now and
>> >>>> I
>> >>>> would be very hesitant to make a change breaking backwards
>> >>>> compatibility. 85
>> >>>> failures in the clang repositories themselves sounds scary to me. Not
>> >>>> saying
>> >>>> we shouldn't do it at all. But we should be very clear on where
>> >>>> things
>> >>>> should be in the long run.
>> >>>
>> >>>
>> >>> Aaron has clarified that that's only 14 outside the AST matcher tests
>> >>> themselves.
>> >>
>> >>
>> >> Sure, still a lot IMO :(. But ok, maybe there is just no other way.
>>
>> There was one additional file with two instances that was sneakily
>> hiding in my output, so the total count is 16.
>>
>> > I think the trade-off is expected confusion this causes in the future,
>> > and
>> > thus people wasting time, vs. the on-time cost of migrating.
>>
>> FWIW (as someone who has been writing a lot of matcher code lately for
>> C and C++), I've never been confused by ctorInitializer() instead of
>> cxxCtorInitializer() (et al), but I spent an embarrassing amount of
>> time learning that recordDecl() meant CXXRecordDecl instead of
>> RecordDecl. Have we ever documented the AST matchers as a stable API?
>> I guess I've always assumed they were like the rest of LLVM's C++ APIs
>> -- you can use them, but sometimes you need to migrate your code
>> because we make API changes.
>>
>> I think that for the cases where there is no corresponding unprefixed
>> version, the chances of getting confused are low. However, I do
>> slightly worry that unprefixed versions may conflict with future
>> language features in C, but not to the point I would lose sleep. If
>> we're talking about breaking people's code to be consistent with the
>> AST node nomenclature, we *could* break it consistently everywhere (a
>> lot of pain, but only one-time pain in theory). However, does that
>> then imply we cannot change AST node names because it would break this
>> mapping? In the case of RecordDecl/CXXRecordDecl, there's a definite
>> pain point for matcher usability. In the other cases, I think the
>> return we receive on breaking people's code is considerably less
>> valuable, but I've not done an exhaustive search of name mappings. If
>> people think that's of value, I could do that (it may also give us a
>> good idea of where we have AST nodes that aren't currently matchable).
>>
>> ~Aaron
>>
>> > Richard, do we plan to remove the CXX prefix from nodes that do not have
>> > non-CXX versions?
>> >
>> >
>> >>
>> >>
>> >>>>
>> >>>>
>> >>>> On Mon, Sep 14, 2015 at 7:03 PM, Aaron Ballman
>> >>>> <aaron at aaronballman.com>
>> >>>> wrote:
>> >>>>>
>> >>>>> On Mon, Sep 14, 2015 at 11:49 AM, Manuel Klimek <klimek at google.com>
>> >>>>> wrote:
>> >>>>> >
>> >>>>> >
>> >>>>> > On Mon, Sep 14, 2015, 8:40 AM Aaron Ballman
>> >>>>> > <aaron at aaronballman.com>
>> >>>>> > wrote:
>> >>>>> >>
>> >>>>> >> On Sat, Sep 12, 2015 at 11:06 PM, Manuel Klimek
>> >>>>> >> <klimek at google.com>
>> >>>>> >> wrote:
>> >>>>> >> >
>> >>>>> >> >
>> >>>>> >> > On Sat, Sep 12, 2015, 9:25 PM Aaron Ballman
>> >>>>> >> > <aaron at aaronballman.com>
>> >>>>> >> > wrote:
>> >>>>> >> >>
>> >>>>> >> >> On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek
>> >>>>> >> >> <klimek at google.com>
>> >>>>> >> >> wrote:
>> >>>>> >> >> >
>> >>>>> >> >> >
>> >>>>> >> >> > 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.
>> >>>>> >> >>
>> >>>>> >> >> Personally, I think this makes the most sense, at least to me.
>> >>>>> >> >> The
>> >>>>> >> >> recommendation I've always heard (and given) is to use
>> >>>>> >> >> -ast-dump
>> >>>>> >> >> and
>> >>>>> >> >> write matchers from there. (Consequently, the more I work with
>> >>>>> >> >> type
>> >>>>> >> >> traversal matchers, the more I wish we had -ast-dump-types to
>> >>>>> >> >> give
>> >>>>> >> >> even *more* information for writing matchers.)
>> >>>>> >> >>
>> >>>>> >> >> But the question still remains, what do we do with recordDecl?
>> >>>>> >> >> Right
>> >>>>> >> >> now, it means CXXRecordDecl instead of RecordDecl. If we
>> >>>>> >> >> change
>> >>>>> >> >> it to
>> >>>>> >> >> mean RecordDecl instead, there's a chance we'll break
>> >>>>> >> >> existing,
>> >>>>> >> >> reasonable code. Are we okay with that risk? If we're at least
>> >>>>> >> >> conceptually okay with it, I could make the change locally and
>> >>>>> >> >> see
>> >>>>> >> >> just how much of our own code breaks, and report back. But if
>> >>>>> >> >> that
>> >>>>> >> >> turns out to be problematic, do we want to deprecate
>> >>>>> >> >> recordDecl
>> >>>>> >> >> and
>> >>>>> >> >> replace it with structDecl as our fallback position? Or is
>> >>>>> >> >> there
>> >>>>> >> >> a
>> >>>>> >> >> better solution?
>> >>>>> >> >>
>> >>>>> >> >> Basically, I see a few ways to solve this (and there may be
>> >>>>> >> >> other
>> >>>>> >> >> ways
>> >>>>> >> >> I'm not thinking about yet):
>> >>>>> >> >>
>> >>>>> >> >> 1) Undocument/deprecate recordDecl, add structDecl, unionDecl,
>> >>>>> >> >> and
>> >>>>> >> >> cxxRecordDecl. This does not match the AST because we have no
>> >>>>> >> >> StructDecl or UnionDecl types. The more I think about this
>> >>>>> >> >> option, the
>> >>>>> >> >> less I like it. It's easy to implement, but seems hard to
>> >>>>> >> >> relate
>> >>>>> >> >> to
>> >>>>> >> >> the AST.
>> >>>>> >> >> 2) Make recordDecl match RecordDecl, don't touch other
>> >>>>> >> >> matchers.
>> >>>>> >> >> Add
>> >>>>> >> >> way to distinguish unions from structs (e.g., isUnion(),
>> >>>>> >> >> isStruct()),
>> >>>>> >> >> add cxxRecordDecl. This matches the AST most closely, but may
>> >>>>> >> >> break
>> >>>>> >> >> code. I think that I prefer this approach, but it depends
>> >>>>> >> >> heavily
>> >>>>> >> >> on
>> >>>>> >> >> what "may break code" looks like in practice.
>> >>>>> >> >> 3) Make recordDecl match RecordDecl, fix other matchers that
>> >>>>> >> >> currently
>> >>>>> >> >> take CXXRecordDecl to instead take RecordDecl and handle
>> >>>>> >> >> sensibly
>> >>>>> >> >> when
>> >>>>> >> >> possible. Add a way to distinguish unions from structs, add
>> >>>>> >> >> cxxRecordDecl. This doesn't match the AST because we will have
>> >>>>> >> >> matchers taking a RecordDecl when the AST would require a
>> >>>>> >> >> CXXRecordDecl, but is likely to break less code.
>> >>>>> >> >
>> >>>>> >> >
>> >>>>> >> > That sums it up. My preferences are 2, 3, 1 in that order :)
>> >>>>> >>
>> >>>>> >> I've attached a patch that implements #2, but it comes with ~85
>> >>>>> >> errors
>> >>>>> >> from C++ matchers that use recordDecl to mean cxxRecordDecl.
>> >>>>> >>
>> >>>>> >> http://pastebin.com/bxkRcqBV
>> >>>>> >>
>> >>>>> >> If this is an acceptable failure rate, I can also update the
>> >>>>> >> failing
>> >>>>> >> matchers to use cxxRecordDecl instead of recordDecl where
>> >>>>> >> applicable.
>> >>>>> >> Doing some spot-checking of the failing code, the failures are
>> >>>>> >> ones
>> >>>>> >> we
>> >>>>> >> anticipated, such as:
>> >>>>> >>
>> >>>>> >> constructorDecl(ofClass(recordDecl(
>> >>>>> >> hasDeclaration(recordDecl(hasMethod(hasName("begin")),
>> >>>>> >> hasMethod(hasName("end"))))
>> >>>>> >> etc
>> >>>>> >
>> >>>>> >
>> >>>>> > +Daniel for another opinion. I think this is fine, but I'd  prefer
>> >>>>> > not to
>> >>>>> > end up in a meme :)
>> >>>>>
>> >>>>> FWIW, the vast majority of the errors are in ASTMatchersTests. There
>> >>>>> were only a few in actual real-world uses (71 in tests, 14 in real
>> >>>>> code).
>> >>>>>
>> >>>>> ~Aaron
>> >>>>>
>> >>>>> >
>> >>>>> >> ~Aaron
>> >>>>> >>
>> >>>>> >> >
>> >>>>> >> >>
>> >>>>> >> >> ~Aaron
>> >>>>> >> >>
>> >>>>> >> >>
>> >>>>> >> >> > 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
>> >>>>> >> >> >> >
>> >>>>> >> >> >> >
>> >>>>
>> >>>>
>> >
>
>