recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

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