Re: recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

Quick ping. I know this is a fairly gigantic patch, but I'm hoping for
a relatively quick review turnaround because of potential merge
conflicts with people doing a fair amount of work on clang-tidy
lately. Everything should be pretty straight-forward (it's all just
renames, no semantic changes intended aside from
recordDecl/cxxRecordDecl and the narrowing matchers.

~Aaron

On Tue, Sep 15, 2015 at 1:32 PM, Aaron Ballman  wrote:
> Here are the complete patches to solve the issues we've discussed:
>
> 1) splits recordDecl into recordDecl and cxxRecordDecl
> 2) adds isStruct, isUnion, isClass to identify what kind of
> recordDecl() you may be looking at
> 3) prefixes all of the node matchers with cxx that should have it
> 4) fixes a similar issue with CUDAKernelCallExpr (the prefix needs to
> be cuda instead of CUDA to distinguish the matcher name from the type
> name).
> 5) updates all of the documentation and code that broke.
>
> One patch is for changes to clang, the other is for changes to
> clang-tools-extra.
>
> ~Aaron
>
> On Mon, Sep 14, 2015 at 5:49 PM, Aaron Ballman  wrote:
>> On Mon, Sep 14, 2015 at 5:47 PM, Daniel Jasper  wrote:
>>> Btw, I think generating them, potentially into several different headers to
>>> work around the compile time issue isn't such a bad idea.
>>
>> I'm not going to start with this approach, but think it may be worth
>> exploring at some point. ;-)
>>
>> ~Aaron
>>
>>>
>>> On Mon, Sep 14, 2015 at 11:45 PM, Manuel Klimek  wrote:

 Feel free to rename the AST nodes :)


 On Mon, Sep 14, 2015, 2:44 PM Daniel Jasper  wrote:
>
> Ok. I am happy with this then.
>
> (Just personally grumpy having to write
> cxxRecordDecl(has(cxxConstructorDecl(..))) in the future ;-) ).
>
> On Mon, Sep 14, 2015 at 11:41 PM, Manuel Klimek 
> wrote:
>>
>>
>>
>> On Mon, Sep 14, 2015 at 2:29 PM Aaron Ballman 
>> wrote:
>>>
>>> On Mon, Sep 14, 2015 at 4:38 PM, Manuel Klimek 
>>> wrote:
>>> >
>>> >
>>> > On Mon, Sep 14, 2015 at 12:26 PM Aaron Ballman
>>> > 
>>> > wrote:
>>> >>
>>> >> On Mon, Sep 14, 2015 at 2:45 PM, Daniel Jasper 
>>> >> 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 

Re: recordDecl() AST matcher

[Richard Smith via cfe-commits]

On Mon, Sep 14, 2015 at 2:41 PM, Manuel Klimek  wrote:
>
> On Mon, Sep 14, 2015 at 2:29 PM Aaron Ballman 
> wrote:
>
>> On Mon, Sep 14, 2015 at 4:38 PM, Manuel Klimek  wrote:
>> >
>> >
>> > On Mon, Sep 14, 2015 at 12:26 PM Aaron Ballman 
>> > wrote:
>> >>
>> >> On Mon, Sep 14, 2015 at 2:45 PM, Daniel Jasper 
>> 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.
>>
>> I can get behind that logic. So we're okay with breaking their code
>> because there's no way around it -- it's tied to the AST, so users
>> cannot rely on the AST APIs remaining the same from release to release
>> anyway.
>>
>
> We might even *want* the code to break, as the use of the AST node might
> now be incorrect on a semantic level.
>
>
>>
>> >
>> >>
>> >> > 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
>> :(
>>
>> Oh, that's an excellent point (I'm talking about (b), I already knew
>> (a) was a lot of work). Thank you for pointing that out!
>>
>> >
>> >>
>> >> My definition of "incredibly obvious" is: if the AST has a prefixed
>> >> and 

Re: recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

On Sat, Sep 12, 2015 at 11:06 PM, Manuel Klimek  wrote:
>
>
> On Sat, Sep 12, 2015, 9:25 PM Aaron Ballman  wrote:
>>
>> On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek  wrote:
>> >
>> >
>> > On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman 
>> > wrote:
>> >>
>> >> On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith 
>> >> 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

~Aaron

>
>>
>> ~Aaron
>>
>>
>> > Richard: if CXXRecordDecl was really an 

Re: recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

On Mon, Sep 14, 2015, 8:40 AM Aaron Ballman  wrote:

> On Sat, Sep 12, 2015 at 11:06 PM, Manuel Klimek  wrote:
> >
> >
> > On Sat, Sep 12, 2015, 9:25 PM Aaron Ballman 
> wrote:
> >>
> >> On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek 
> wrote:
> >> >
> >> >
> >> > On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman <
> aa...@aaronballman.com>
> >> > wrote:
> >> >>
> >> >> On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith <
> rich...@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.

Re: recordDecl() AST matcher

[Daniel Jasper via cfe-commits]

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

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.

On Mon, Sep 14, 2015 at 7:03 PM, Aaron Ballman 
wrote:

> On Mon, Sep 14, 2015 at 11:49 AM, Manuel Klimek  wrote:
> >
> >
> > On Mon, Sep 14, 2015, 8:40 AM Aaron Ballman 
> wrote:
> >>
> >> On Sat, Sep 12, 2015 at 11:06 PM, Manuel Klimek 
> wrote:
> >> >
> >> >
> >> > On Sat, Sep 12, 2015, 9:25 PM Aaron Ballman 
> >> > wrote:
> >> >>
> >> >> On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek 
> >> >> wrote:
> >> >> >
> >> >> >
> >> >> > On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman
> >> >> > 
> >> >> > wrote:
> >> >> >>
> >> >> >> On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith
> >> >> >> 
> >> >> >> 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
> >> >> 

Re: recordDecl() AST matcher

[Daniel Jasper via cfe-commits]

On Mon, Sep 14, 2015 at 7:24 PM, Manuel Klimek  wrote:

> On Mon, Sep 14, 2015 at 10:21 AM Daniel Jasper  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.


>
>> On Mon, Sep 14, 2015 at 7:03 PM, Aaron Ballman 
>> wrote:
>>
>>> On Mon, Sep 14, 2015 at 11:49 AM, Manuel Klimek 
>>> wrote:
>>> >
>>> >
>>> > On Mon, Sep 14, 2015, 8:40 AM Aaron Ballman 
>>> wrote:
>>> >>
>>> >> On Sat, Sep 12, 2015 at 11:06 PM, Manuel Klimek 
>>> wrote:
>>> >> >
>>> >> >
>>> >> > On Sat, Sep 12, 2015, 9:25 PM Aaron Ballman >> >
>>> >> > wrote:
>>> >> >>
>>> >> >> On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek 
>>> >> >> wrote:
>>> >> >> >
>>> >> >> >
>>> >> >> > On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman
>>> >> >> > 
>>> >> >> > wrote:
>>> >> >> >>
>>> >> >> >> On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith
>>> >> >> >> 
>>> >> >> >> 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 

Re: recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

On Mon, Sep 14, 2015 at 10:30 AM Daniel Jasper  wrote:

> On Mon, Sep 14, 2015 at 7:24 PM, Manuel Klimek  wrote:
>
>> On Mon, Sep 14, 2015 at 10:21 AM Daniel Jasper 
>> 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 
>>> wrote:
>>>
 On Mon, Sep 14, 2015 at 11:49 AM, Manuel Klimek 
 wrote:
 >
 >
 > On Mon, Sep 14, 2015, 8:40 AM Aaron Ballman 
 wrote:
 >>
 >> On Sat, Sep 12, 2015 at 11:06 PM, Manuel Klimek 
 wrote:
 >> >
 >> >
 >> > On Sat, Sep 12, 2015, 9:25 PM Aaron Ballman <
 aa...@aaronballman.com>
 >> > wrote:
 >> >>
 >> >> On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek 
 >> >> wrote:
 >> >> >
 >> >> >
 >> >> > On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman
 >> >> > 
 >> >> > wrote:
 >> >> >>
 >> >> >> On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith
 >> >> >> 
 >> >> >> 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 

Re: recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

This is the full patch that corrects all the compile errors MSVC was
generating. If we have any platform-specific checkers, they may have
been missed. But this should give an idea of the scope of the changes
we're asking folks to make.

~Aaron

On Mon, Sep 14, 2015 at 1:03 PM, Aaron Ballman  wrote:
> On Mon, Sep 14, 2015 at 11:49 AM, Manuel Klimek  wrote:
>>
>>
>> On Mon, Sep 14, 2015, 8:40 AM Aaron Ballman  wrote:
>>>
>>> On Sat, Sep 12, 2015 at 11:06 PM, Manuel Klimek  wrote:
>>> >
>>> >
>>> > On Sat, Sep 12, 2015, 9:25 PM Aaron Ballman 
>>> > wrote:
>>> >>
>>> >> On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek 
>>> >> wrote:
>>> >> >
>>> >> >
>>> >> > On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman
>>> >> > 
>>> >> > wrote:
>>> >> >>
>>> >> >> On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith
>>> >> >> 
>>> >> >> 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 

Re: recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

On Mon, Sep 14, 2015 at 10:21 AM Daniel Jasper  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.

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.


>
> On Mon, Sep 14, 2015 at 7:03 PM, Aaron Ballman 
> wrote:
>
>> On Mon, Sep 14, 2015 at 11:49 AM, Manuel Klimek 
>> wrote:
>> >
>> >
>> > On Mon, Sep 14, 2015, 8:40 AM Aaron Ballman 
>> wrote:
>> >>
>> >> On Sat, Sep 12, 2015 at 11:06 PM, Manuel Klimek 
>> wrote:
>> >> >
>> >> >
>> >> > On Sat, Sep 12, 2015, 9:25 PM Aaron Ballman 
>> >> > wrote:
>> >> >>
>> >> >> On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek 
>> >> >> wrote:
>> >> >> >
>> >> >> >
>> >> >> > On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman
>> >> >> > 
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith
>> >> >> >> 
>> >> >> >> 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 

Re: recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

On Mon, Sep 14, 2015 at 5:44 PM, Daniel Jasper  wrote:
> Ok. I am happy with this then.
>
> (Just personally grumpy having to write
> cxxRecordDecl(has(cxxConstructorDecl(..))) in the future ;-) ).

I share your grumpiness about the cxxConstructorDecl, but probably
won't share it when we add objcConstructorDecl or some crazy thing.
:-P

I will get started on this and come back with a patch.

Thank you all for the conversation!

~Aaron

>
> On Mon, Sep 14, 2015 at 11:41 PM, Manuel Klimek  wrote:
>>
>>
>>
>> On Mon, Sep 14, 2015 at 2:29 PM Aaron Ballman 
>> wrote:
>>>
>>> On Mon, Sep 14, 2015 at 4:38 PM, Manuel Klimek  wrote:
>>> >
>>> >
>>> > On Mon, Sep 14, 2015 at 12:26 PM Aaron Ballman 
>>> > wrote:
>>> >>
>>> >> On Mon, Sep 14, 2015 at 2:45 PM, Daniel Jasper 
>>> >> 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.
>>>
>>> I can get behind that logic. So we're okay with breaking their code
>>> because there's no way around it -- it's tied to the AST, so users
>>> cannot rely on the AST APIs remaining the same from release to release
>>> anyway.
>>
>>
>> We might even *want* the code to break, as the use of the AST node might
>> now be incorrect on a semantic level.
>>
>>>
>>>
>>> >
>>> >>
>>> >> > 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 

Re: recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

On Mon, Sep 14, 2015 at 2:29 PM Aaron Ballman 
wrote:

> On Mon, Sep 14, 2015 at 4:38 PM, Manuel Klimek  wrote:
> >
> >
> > On Mon, Sep 14, 2015 at 12:26 PM Aaron Ballman 
> > wrote:
> >>
> >> On Mon, Sep 14, 2015 at 2:45 PM, Daniel Jasper 
> 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.
>
> I can get behind that logic. So we're okay with breaking their code
> because there's no way around it -- it's tied to the AST, so users
> cannot rely on the AST APIs remaining the same from release to release
> anyway.
>

We might even *want* the code to break, as the use of the AST node might
now be incorrect on a semantic level.


>
> >
> >>
> >> > 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
> :(
>
> Oh, that's an excellent point (I'm talking about (b), I already knew
> (a) was a lot of work). Thank you for pointing that out!
>
> >
> >>
> >> 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
> >> 

Re: recordDecl() AST matcher

[Daniel Jasper via cfe-commits]

Btw, I think generating them, potentially into several different headers to
work around the compile time issue isn't such a bad idea.

On Mon, Sep 14, 2015 at 11:45 PM, Manuel Klimek  wrote:

> Feel free to rename the AST nodes :)
>
> On Mon, Sep 14, 2015, 2:44 PM Daniel Jasper  wrote:
>
>> Ok. I am happy with this then.
>>
>> (Just personally grumpy having to write
>> cxxRecordDecl(has(cxxConstructorDecl(..))) in the future ;-) ).
>>
>> On Mon, Sep 14, 2015 at 11:41 PM, Manuel Klimek 
>> wrote:
>>
>>>
>>>
>>> On Mon, Sep 14, 2015 at 2:29 PM Aaron Ballman 
>>> wrote:
>>>
 On Mon, Sep 14, 2015 at 4:38 PM, Manuel Klimek 
 wrote:
 >
 >
 > On Mon, Sep 14, 2015 at 12:26 PM Aaron Ballman <
 aa...@aaronballman.com>
 > wrote:
 >>
 >> On Mon, Sep 14, 2015 at 2:45 PM, Daniel Jasper 
 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.

 I can get behind that logic. So we're okay with breaking their code
 because there's no way around it -- it's tied to the AST, so users
 cannot rely on the AST APIs remaining the same from release to release
 anyway.

>>>
>>> We might even *want* the code to break, as the use of the AST node might
>>> now be incorrect on a semantic level.
>>>
>>>

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

Re: recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

On Mon, Sep 14, 2015 at 1:37 PM, Manuel Klimek  wrote:
> On Mon, Sep 14, 2015 at 10:30 AM Daniel Jasper  wrote:
>>
>> On Mon, Sep 14, 2015 at 7:24 PM, Manuel Klimek  wrote:
>>>
>>> On Mon, Sep 14, 2015 at 10:21 AM Daniel Jasper 
>>> 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 
 wrote:
>
> On Mon, Sep 14, 2015 at 11:49 AM, Manuel Klimek 
> wrote:
> >
> >
> > On Mon, Sep 14, 2015, 8:40 AM Aaron Ballman 
> > wrote:
> >>
> >> On Sat, Sep 12, 2015 at 11:06 PM, Manuel Klimek 
> >> wrote:
> >> >
> >> >
> >> > On Sat, Sep 12, 2015, 9:25 PM Aaron Ballman
> >> > 
> >> > wrote:
> >> >>
> >> >> On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek
> >> >> 
> >> >> wrote:
> >> >> >
> >> >> >
> >> >> > On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman
> >> >> > 
> >> >> > wrote:
> >> >> >>
> >> >> >> On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith
> >> >> >> 
> >> >> >> 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
> >> >> >> 

Re: recordDecl() AST matcher

[Daniel Jasper via cfe-commits]

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?
2) Do we want to be consistent and change the other 27 matchers as well?

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


On Mon, Sep 14, 2015 at 8:08 PM, Aaron Ballman 
wrote:

> On Mon, Sep 14, 2015 at 1:37 PM, Manuel Klimek  wrote:
> > On Mon, Sep 14, 2015 at 10:30 AM Daniel Jasper 
> wrote:
> >>
> >> On Mon, Sep 14, 2015 at 7:24 PM, Manuel Klimek 
> wrote:
> >>>
> >>> On Mon, Sep 14, 2015 at 10:21 AM Daniel Jasper 
> >>> 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 

Re: recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

On Mon, Sep 14, 2015 at 11:45 AM Daniel Jasper  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?
>

Yes. Basically every user of this I've talked to complains about this
(point in case: sitting next to jdennett, and while talking about this with
Richard he jumped in with "yes, me too").


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

I think we do want to change the rest, too. After talking with Richard, I
think the probability that we'll change them (in the short/mid term) is low.
I do think changing the rest has lower priority than changing recordDecl of
course.


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

I think the AST matchers have multiple layers, and the lowest layer should
match the AST 1:1 (from feedback I've gotten from users).
There are and will be higher level layers on top of it, which are also AST
matchers, but which abstract from the lower level matchers to some degree.
Perhaps we should make that more clear in how we lay out matchers in the
code.


> 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 ;-).
>
>
> On Mon, Sep 14, 2015 at 8:08 PM, Aaron Ballman 
> wrote:
>
>> On Mon, Sep 14, 2015 at 1:37 PM, Manuel Klimek  wrote:
>> > On Mon, Sep 14, 2015 at 10:30 AM Daniel Jasper 
>> wrote:
>> >>
>> >> On Mon, Sep 14, 2015 at 7:24 PM, Manuel Klimek 
>> wrote:
>> >>>
>> >>> On Mon, Sep 14, 2015 at 10:21 AM Daniel Jasper 
>> >>> 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 

Re: recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

On Mon, Sep 14, 2015 at 2:45 PM, Daniel Jasper  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 
> wrote:
>>
>> On Mon, Sep 14, 2015 at 1:37 PM, Manuel Klimek  wrote:
>> > On Mon, Sep 14, 2015 at 10:30 AM Daniel Jasper 
>> > wrote:
>> >>
>> >> On Mon, Sep 14, 2015 at 7:24 PM, Manuel Klimek 
>> >> wrote:
>> >>>
>> >>> On Mon, Sep 14, 2015 at 10:21 AM Daniel Jasper 
>> >>> 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.
>> >>
>> 

Re: recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

On Sat, Sep 12, 2015, 9:25 PM Aaron Ballman  wrote:

> On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek  wrote:
> >
> >
> > On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman 
> > wrote:
> >>
> >> On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith 
> >> 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 :)


> ~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 
> >> > wrote:
> >> >>
> >> >> Richard! We need an informed opinion :D
> >> >>
> >> >> On Fri, Sep 11, 2015 at 3:07 PM Aaron Ballman <
> aa...@aaronballman.com>
> >> >> wrote:
> >> >>>
> >> >>> Ping?
> >> >>>
> >> >>> On Tue, Sep 8, 2015 at 9:26 AM, Manuel Klimek 
> >> >>> wrote:
> >> >>> > On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman
> 

Re: recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman 
wrote:

> On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith 
> wrote:
> > I don't think CXXRecordDecl is an anachronism, so much as an
> implementation
> > detail; it makes sense to use a smaller class when in C mode, as we don't
> > need most of the features and complexity that CXXRecordDecl brings with
> it.
> > But... as a user of clang matchers, I don't think I'd want to care about
> the
> > difference, and it'd be more convenient if I could nest (say) a hasMethod
> > matcher within a recordDecl matcher, since it's completely obvious what
> that
> > should mean. If I have a matcher that says:
> >
> >   recordDecl(or(hasMethod(...), hasField(...)))
> >
> > I would expect that to work in both C and C++ (and the only way it could
> > match in C would be on a record with the specified field, since the
> > hasMethod matcher would always fail).
>
> Okay, so then it sounds like we want recordDecl to *mean* RecordDecl,
> but we want the traversal and narrowing matchers that currently take a
> CXXRecordDecl to instead take a RecordDecl and handle the CXX part
> transparently? This means we would not need to add a cxxRecordDecl()
> matcher, but could still access CXX-only functionality (like access
> control, base classes, etc) through recordDecl()?
>

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

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


>
> ~Aaron
>
> >
> > On Fri, Sep 11, 2015 at 6:30 AM, Manuel Klimek 
> wrote:
> >>
> >> Richard! We need an informed opinion :D
> >>
> >> On Fri, Sep 11, 2015 at 3:07 PM Aaron Ballman 
> >> wrote:
> >>>
> >>> Ping?
> >>>
> >>> On Tue, Sep 8, 2015 at 9:26 AM, Manuel Klimek 
> wrote:
> >>> > On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman  >
> >>> > wrote:
> >>> >>
> >>> >> On Tue, Sep 8, 2015 at 9:18 AM, Manuel Klimek 
> >>> >> wrote:
> >>> >> > On Tue, Sep 8, 2015 at 2:23 PM Aaron Ballman
> >>> >> > 
> >>> >> > wrote:
> >>> >> >>
> >>> >> >> On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek  >
> >>> >> >> 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
> >>> >> >> > 
> >>> >> >> > 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 

Re: recordDecl() AST matcher

[Richard Smith via cfe-commits]

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

On Fri, Sep 11, 2015 at 6:30 AM, Manuel Klimek  wrote:

> Richard! We need an informed opinion :D
>
> On Fri, Sep 11, 2015 at 3:07 PM Aaron Ballman 
> wrote:
>
>> Ping?
>>
>> On Tue, Sep 8, 2015 at 9:26 AM, Manuel Klimek  wrote:
>> > On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman 
>> wrote:
>> >>
>> >> On Tue, Sep 8, 2015 at 9:18 AM, Manuel Klimek 
>> wrote:
>> >> > On Tue, Sep 8, 2015 at 2:23 PM Aaron Ballman > >
>> >> > wrote:
>> >> >>
>> >> >> On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek 
>> >> >> 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 <
>> aa...@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
>> 

Re: recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith  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()?

~Aaron

>
> On Fri, Sep 11, 2015 at 6:30 AM, Manuel Klimek  wrote:
>>
>> Richard! We need an informed opinion :D
>>
>> On Fri, Sep 11, 2015 at 3:07 PM Aaron Ballman 
>> wrote:
>>>
>>> Ping?
>>>
>>> On Tue, Sep 8, 2015 at 9:26 AM, Manuel Klimek  wrote:
>>> > On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman 
>>> > wrote:
>>> >>
>>> >> On Tue, Sep 8, 2015 at 9:18 AM, Manuel Klimek 
>>> >> wrote:
>>> >> > On Tue, Sep 8, 2015 at 2:23 PM Aaron Ballman
>>> >> > 
>>> >> > wrote:
>>> >> >>
>>> >> >> On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek 
>>> >> >> 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
>>> >> >> > 
>>> >> >> > 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 

Re: recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

Ping?

On Tue, Sep 8, 2015 at 9:26 AM, Manuel Klimek  wrote:
> On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman  wrote:
>>
>> On Tue, Sep 8, 2015 at 9:18 AM, Manuel Klimek  wrote:
>> > On Tue, Sep 8, 2015 at 2:23 PM Aaron Ballman 
>> > wrote:
>> >>
>> >> On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek 
>> >> 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 
>> >> > 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 

Re: recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

Richard! We need an informed opinion :D

On Fri, Sep 11, 2015 at 3:07 PM Aaron Ballman 
wrote:

> Ping?
>
> On Tue, Sep 8, 2015 at 9:26 AM, Manuel Klimek  wrote:
> > On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman 
> wrote:
> >>
> >> On Tue, Sep 8, 2015 at 9:18 AM, Manuel Klimek 
> wrote:
> >> > On Tue, Sep 8, 2015 at 2:23 PM Aaron Ballman 
> >> > wrote:
> >> >>
> >> >> On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek 
> >> >> 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 <
> aa...@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 

Re: recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek  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.

>
> On Fri, Sep 4, 2015 at 8:32 PM Aaron Ballman  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.

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

~Aaron

>
>>
>>
>> Thanks!
>>
>> ~Aaron
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman  wrote:

> On Tue, Sep 8, 2015 at 9:18 AM, Manuel Klimek  wrote:
> > On Tue, Sep 8, 2015 at 2:23 PM Aaron Ballman 
> wrote:
> >>
> >> On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek 
> 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 
> >> > 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 

Re: recordDecl() AST matcher

[Aaron Ballman via cfe-commits]

On Tue, Sep 8, 2015 at 9:18 AM, Manuel Klimek  wrote:
> On Tue, Sep 8, 2015 at 2:23 PM Aaron Ballman  wrote:
>>
>> On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek  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 
>> > 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 

Re: recordDecl() AST matcher

[Manuel Klimek via cfe-commits]

On Tue, Sep 8, 2015 at 2:23 PM Aaron Ballman  wrote:

> On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek  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 
> 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?


>
> ~Aaron
>
> >
> >>
> >>
> >> Thanks!
> >>
> >> ~Aaron
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits