recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

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

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.

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
>>>> >> >> >> >
>>>> >> >> >> >
>>>>
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150914/870ac466/attachment-0001.html>