recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

On Mon, Sep 14, 2015 at 12:26 PM Aaron Ballman <aaron at aaronballman.com>
wrote:

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

I think the AST matchers are so closely coupled to the AST that it trying
to be more stable than the AST doesn't help. Basically all uses of AST
matchers do something with the AST nodes afterwards, which will break
anyway.


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

I do think we want to auto-generate the matchers, but I don't think
tablegen is the right approach (I think an ast-matcher based tool is ;)
That said, auto-generating all the matchers is a) a lot of effort and b)
the code-size and compile time of matchers already matters, so it's unclear
which ones we would want to generate, especially for traversal matchers :(


> 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 think the main things is cost for developers who try to write matchers
and work from the -ast-dump. Figuring out that there *is* a matcher with an
unprefixed node can take a while.


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

Well, I think there's the cost of broken code *once* now, vs. the (smaller)
cost for users in all future.
I'm still strongly in favor of breaking now, and having a simpler model
going forward.


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