<br><br><div class="gmail_quote"><div dir="ltr">On Mon, Sep 14, 2015, 8:40 AM Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Sat, Sep 12, 2015 at 11:06 PM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:<br>
><br>
><br>
> On Sat, Sep 12, 2015, 9:25 PM Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>> wrote:<br>
>><br>
>> On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> > On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>><br>
>> >> wrote:<br>
>> >> > I don't think CXXRecordDecl is an anachronism, so much as an<br>
>> >> > implementation<br>
>> >> > detail; it makes sense to use a smaller class when in C mode, as we<br>
>> >> > don't<br>
>> >> > need most of the features and complexity that CXXRecordDecl brings<br>
>> >> > with<br>
>> >> > it.<br>
>> >> > But... as a user of clang matchers, I don't think I'd want to care<br>
>> >> > about<br>
>> >> > the<br>
>> >> > difference, and it'd be more convenient if I could nest (say) a<br>
>> >> > hasMethod<br>
>> >> > matcher within a recordDecl matcher, since it's completely obvious<br>
>> >> > what<br>
>> >> > that<br>
>> >> > should mean. If I have a matcher that says:<br>
>> >> ><br>
>> >> >   recordDecl(or(hasMethod(...), hasField(...)))<br>
>> >> ><br>
>> >> > I would expect that to work in both C and C++ (and the only way it<br>
>> >> > could<br>
>> >> > match in C would be on a record with the specified field, since the<br>
>> >> > hasMethod matcher would always fail).<br>
>> >><br>
>> >> Okay, so then it sounds like we want recordDecl to *mean* RecordDecl,<br>
>> >> but we want the traversal and narrowing matchers that currently take a<br>
>> >> CXXRecordDecl to instead take a RecordDecl and handle the CXX part<br>
>> >> transparently? This means we would not need to add a cxxRecordDecl()<br>
>> >> matcher, but could still access CXX-only functionality (like access<br>
>> >> control, base classes, etc) through recordDecl()?<br>
>> ><br>
>> ><br>
>> > I'm against that proposal. I think we have tried to make the matchers<br>
>> > more<br>
>> > "user friendly" in the past, and all those attempts have failed<br>
>> > miserably;<br>
>> > in the end, users will do ast-dump to see what they want to match, and<br>
>> > then<br>
>> > be confused when the matchers do follow the AST 99% of the time, but try<br>
>> > to<br>
>> > be smart 1% of the time.<br>
>> > I think given that we want to keep CXXRecordDecl, the right solution is<br>
>> > to<br>
>> > have a cxxRecordDecl() matcher.<br>
>><br>
>> Personally, I think this makes the most sense, at least to me. The<br>
>> recommendation I've always heard (and given) is to use -ast-dump and<br>
>> write matchers from there. (Consequently, the more I work with type<br>
>> traversal matchers, the more I wish we had -ast-dump-types to give<br>
>> even *more* information for writing matchers.)<br>
>><br>
>> But the question still remains, what do we do with recordDecl? Right<br>
>> now, it means CXXRecordDecl instead of RecordDecl. If we change it to<br>
>> mean RecordDecl instead, there's a chance we'll break existing,<br>
>> reasonable code. Are we okay with that risk? If we're at least<br>
>> conceptually okay with it, I could make the change locally and see<br>
>> just how much of our own code breaks, and report back. But if that<br>
>> turns out to be problematic, do we want to deprecate recordDecl and<br>
>> replace it with structDecl as our fallback position? Or is there a<br>
>> better solution?<br>
>><br>
>> Basically, I see a few ways to solve this (and there may be other ways<br>
>> I'm not thinking about yet):<br>
>><br>
>> 1) Undocument/deprecate recordDecl, add structDecl, unionDecl, and<br>
>> cxxRecordDecl. This does not match the AST because we have no<br>
>> StructDecl or UnionDecl types. The more I think about this option, the<br>
>> less I like it. It's easy to implement, but seems hard to relate to<br>
>> the AST.<br>
>> 2) Make recordDecl match RecordDecl, don't touch other matchers. Add<br>
>> way to distinguish unions from structs (e.g., isUnion(), isStruct()),<br>
>> add cxxRecordDecl. This matches the AST most closely, but may break<br>
>> code. I think that I prefer this approach, but it depends heavily on<br>
>> what "may break code" looks like in practice.<br>
>> 3) Make recordDecl match RecordDecl, fix other matchers that currently<br>
>> take CXXRecordDecl to instead take RecordDecl and handle sensibly when<br>
>> possible. Add a way to distinguish unions from structs, add<br>
>> cxxRecordDecl. This doesn't match the AST because we will have<br>
>> matchers taking a RecordDecl when the AST would require a<br>
>> CXXRecordDecl, but is likely to break less code.<br>
><br>
><br>
> That sums it up. My preferences are 2, 3, 1 in that order :)<br>
<br>
I've attached a patch that implements #2, but it comes with ~85 errors<br>
from C++ matchers that use recordDecl to mean cxxRecordDecl.<br>
<br>
<a href="http://pastebin.com/bxkRcqBV" rel="noreferrer" target="_blank">http://pastebin.com/bxkRcqBV</a><br>
<br>
If this is an acceptable failure rate, I can also update the failing<br>
matchers to use cxxRecordDecl instead of recordDecl where applicable.<br>
Doing some spot-checking of the failing code, the failures are ones we<br>
anticipated, such as:<br>
<br>
constructorDecl(ofClass(recordDecl(<br>
hasDeclaration(recordDecl(hasMethod(hasName("begin")),<br>
hasMethod(hasName("end"))))<br>
etc<br></blockquote></div><div><br></div><div>+Daniel for another opinion. I think this is fine, but I'd  prefer not to end up in a meme :) </div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
~Aaron<br>
<br>
><br>
>><br>
>> ~Aaron<br>
>><br>
>><br>
>> > Richard: if CXXRecordDecl was really an implementation detail, it would<br>
>> > be<br>
>> > hidden behind a RecordDecl class, as an implementation detail. The<br>
>> > reasons<br>
>> > why we don't want it to be an implementation detail in the code<br>
>> > (performance, data structure size) don't matter - in the end, it's in<br>
>> > the<br>
>> > AST API.<br>
>> ><br>
>> >><br>
>> >><br>
>> >> ~Aaron<br>
>> >><br>
>> >> ><br>
>> >> > On Fri, Sep 11, 2015 at 6:30 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>><br>
>> >> > wrote:<br>
>> >> >><br>
>> >> >> Richard! We need an informed opinion :D<br>
>> >> >><br>
>> >> >> On Fri, Sep 11, 2015 at 3:07 PM Aaron Ballman<br>
>> >> >> <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>><br>
>> >> >> wrote:<br>
>> >> >>><br>
>> >> >>> Ping?<br>
>> >> >>><br>
>> >> >>> On Tue, Sep 8, 2015 at 9:26 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>><br>
>> >> >>> wrote:<br>
>> >> >>> > On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman<br>
>> >> >>> > <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>><br>
>> >> >>> > wrote:<br>
>> >> >>> >><br>
>> >> >>> >> On Tue, Sep 8, 2015 at 9:18 AM, Manuel Klimek<br>
>> >> >>> >> <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>><br>
>> >> >>> >> wrote:<br>
>> >> >>> >> > On Tue, Sep 8, 2015 at 2:23 PM Aaron Ballman<br>
>> >> >>> >> > <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>><br>
>> >> >>> >> > wrote:<br>
>> >> >>> >> >><br>
>> >> >>> >> >> On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek<br>
>> >> >>> >> >> <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>><br>
>> >> >>> >> >> wrote:<br>
>> >> >>> >> >> > Yea, we had that discussion a few times, and I can never<br>
>> >> >>> >> >> > remember<br>
>> >> >>> >> >> > why<br>
>> >> >>> >> >> > we<br>
>> >> >>> >> >> > ended up in the state we're in.<br>
>> >> >>> >> >> > We definitely had a time where we switched to just using<br>
>> >> >>> >> >> > the<br>
>> >> >>> >> >> > exact<br>
>> >> >>> >> >> > same<br>
>> >> >>> >> >> > name<br>
>> >> >>> >> >> > as the node's class name for the matchers.<br>
>> >> >>> >> >> > I *think* we didn't do it for cxxRecordDecl, because<br>
>> >> >>> >> >> > Richard<br>
>> >> >>> >> >> > said<br>
>> >> >>> >> >> > that's<br>
>> >> >>> >> >> > a<br>
>> >> >>> >> >> > relic we should get rid of anyway, but I'm not sure.<br>
>> >> >>> >> >><br>
>> >> >>> >> >> FWIW, I think the state we're in is the worst of all worlds.<br>
>> >> >>> >> >> It's<br>
>> >> >>> >> >> not<br>
>> >> >>> >> >> intuitive that recordDecl() doesn't match a struct in C mode,<br>
>> >> >>> >> >> and<br>
>> >> >>> >> >> as<br>
>> >> >>> >> >> it stands, there is no way to match a struct or union<br>
>> >> >>> >> >> declaration<br>
>> >> >>> >> >> in C<br>
>> >> >>> >> >> at all.<br>
>> >> >>> >> ><br>
>> >> >>> >> ><br>
>> >> >>> >> > Agreed. Best intentions. Worst possible outcome. That's<br>
>> >> >>> >> > software<br>
>> >> >>> >> > development<br>
>> >> >>> >> > :)<br>
>> >> >>> >> ><br>
>> >> >>> >> >> ><br>
>> >> >>> >> >> > On Fri, Sep 4, 2015 at 8:32 PM Aaron Ballman<br>
>> >> >>> >> >> > <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>><br>
>> >> >>> >> >> > wrote:<br>
>> >> >>> >> >> >><br>
>> >> >>> >> >> >> It turns out that the recordDecl() AST matcher doesn't<br>
>> >> >>> >> >> >> match<br>
>> >> >>> >> >> >> RecordDecl objects; instead, it matches CXXRecordDecl<br>
>> >> >>> >> >> >> objects.<br>
>> >> >>> >> >> >> This<br>
>> >> >>> >> >> >> is... unfortunate... as it makes writing AST matchers more<br>
>> >> >>> >> >> >> complicated<br>
>> >> >>> >> >> >> because of having to translate between<br>
>> >> >>> >> >> >> recordDecl()/CXXRecordDecl.<br>
>> >> >>> >> >> >> It<br>
>> >> >>> >> >> >> also makes it impossible to match a struct or union<br>
>> >> >>> >> >> >> declaration<br>
>> >> >>> >> >> >> in C<br>
>> >> >>> >> >> >> or ObjC. However, given how prevalent recordDecl()'s use<br>
>> >> >>> >> >> >> is<br>
>> >> >>> >> >> >> in<br>
>> >> >>> >> >> >> the<br>
>> >> >>> >> >> >> wild (I'm guessing), changing it at this point would be a<br>
>> >> >>> >> >> >> Bad<br>
>> >> >>> >> >> >> Thing.<br>
>> >> >>> >> >> >><br>
>> >> >>> >> >> >> For people trying to write AST matchers for languages like<br>
>> >> >>> >> >> >> C<br>
>> >> >>> >> >> >> or<br>
>> >> >>> >> >> >> ObjC,<br>
>> >> >>> >> >> >> I would like to propose adding:<br>
>> >> >>> >> >> >><br>
>> >> >>> >> >> >> structDecl()<br>
>> >> >>> >> >> >> unionDecl()<br>
>> >> >>> >> >> >> tagDecl()<br>
>> >> >>> >> >> >><br>
>> >> >>> >> >> >> These will match nicely with the existing enumDecl() AST<br>
>> >> >>> >> >> >> matcher.<br>
>> >> >>> >> >> >><br>
>> >> >>> >> >> >> Additionally, I would like to add cxxRecordDecl() to match<br>
>> >> >>> >> >> >> CXXRecordDecl objects. While it duplicates the<br>
>> >> >>> >> >> >> functionality<br>
>> >> >>> >> >> >> exposed<br>
>> >> >>> >> >> >> by recordDecl(), it more clearly matches the intention of<br>
>> >> >>> >> >> >> which<br>
>> >> >>> >> >> >> AST<br>
>> >> >>> >> >> >> node it corresponds to.<br>
>> >> >>> >> >> >><br>
>> >> >>> >> >> >> Finally, I would like to undocument recordDecl() and<br>
>> >> >>> >> >> >> change<br>
>> >> >>> >> >> >> our<br>
>> >> >>> >> >> >> existing documentation and AST matcher uses to use<br>
>> >> >>> >> >> >> cxxRecordDecl/structDecl() instead. Maybe someday we can<br>
>> >> >>> >> >> >> deprecate<br>
>> >> >>> >> >> >> recordDecl() more officially.<br>
>> >> >>> >> >> >><br>
>> >> >>> >> >> >> I'm open to other ideas if there are better ways to move<br>
>> >> >>> >> >> >> forward. If<br>
>> >> >>> >> >> >> you think changing the meaning of recordDecl() is<br>
>> >> >>> >> >> >> acceptable,<br>
>> >> >>> >> >> >> I<br>
>> >> >>> >> >> >> can<br>
>> >> >>> >> >> >> also go that route (though I would still propose adding<br>
>> >> >>> >> >> >> unionDecl()<br>
>> >> >>> >> >> >> and cxxRecordDecl() in that case).<br>
>> >> >>> >> >> ><br>
>> >> >>> >> >> ><br>
>> >> >>> >> >> > I think changing recordDecl is acceptable. I believe very<br>
>> >> >>> >> >> > few<br>
>> >> >>> >> >> > tools<br>
>> >> >>> >> >> > will<br>
>> >> >>> >> >> > actually start doing wrong things because of it. I'd like<br>
>> >> >>> >> >> > more<br>
>> >> >>> >> >> > opinions<br>
>> >> >>> >> >> > first, though :)<br>
>> >> >>> >> >><br>
>> >> >>> >> >> I was giving this more thought over the long weekend, and I<br>
>> >> >>> >> >> think<br>
>> >> >>> >> >> you<br>
>> >> >>> >> >> may be right. I think changing recordDecl() to mean<br>
>> >> >>> >> >> RecordDecl<br>
>> >> >>> >> >> will<br>
>> >> >>> >> >> fix more code than it breaks, so long as we take a holistic<br>
>> >> >>> >> >> approach<br>
>> >> >>> >> >> to the change and see which narrowing and traversal matchers<br>
>> >> >>> >> >> we<br>
>> >> >>> >> >> need<br>
>> >> >>> >> >> to fix up at the same time. When I tried to think of AST<br>
>> >> >>> >> >> matchers<br>
>> >> >>> >> >> that<br>
>> >> >>> >> >> mean CXXRecordDecl but *not* RecordDecl, they were horribly<br>
>> >> >>> >> >> contrived<br>
>> >> >>> >> >> because you usually are matching on additional selection<br>
>> >> >>> >> >> criteria<br>
>> >> >>> >> >> that<br>
>> >> >>> >> >> is specific to C++ (such as hasMethod() or isDerivedFrom())<br>
>> >> >>> >> >> which<br>
>> >> >>> >> >> would cause the match to continue to fail, as expected. Code<br>
>> >> >>> >> >> that<br>
>> >> >>> >> >> uses<br>
>> >> >>> >> >> recordDecl() to mean RecordDecl will suddenly start to match<br>
>> >> >>> >> >> in<br>
>> >> >>> >> >> more<br>
>> >> >>> >> >> cases, but that's likely to be a bug fix more than a breaking<br>
>> >> >>> >> >> change.<br>
>> >> >>> >> >> To the best of my understanding, the only breaking cases<br>
>> >> >>> >> >> would<br>
>> >> >>> >> >> be<br>
>> >> >>> >> >> where you wrote recordDecl(), meant CXXRecordDecl, had no<br>
>> >> >>> >> >> further<br>
>> >> >>> >> >> narrowing or traversal matchers, and were compiling in C<br>
>> >> >>> >> >> mode;<br>
>> >> >>> >> >> with<br>
>> >> >>> >> >> the result being additional unexpected matches.<br>
>> >> >>> >> ><br>
>> >> >>> >> ><br>
>> >> >>> >> > Ah, there's one thing that can break: the compile can break:<br>
>> >> >>> >> > recordDecl(hasMethod(...)) will *not* compile (it'll work in<br>
>> >> >>> >> > the<br>
>> >> >>> >> > dynamic<br>
>> >> >>> >> > matchers and fail as you suggest, but the in-C++ DSL does more<br>
>> >> >>> >> > static<br>
>> >> >>> >> > type<br>
>> >> >>> >> > checking).<br>
>> >> >>> >> > I don't think that's super bad though.<br>
>> >> >>> >> ><br>
>> >> >>> >> >><br>
>> >> >>> >> >> So perhaps it would make sense to:<br>
>> >> >>> >> >><br>
>> >> >>> >> >> 1) Make recordDecl() mean RecordDecl<br>
>> >> >>> >> >> 2) Do a comprehensive review of matchers that take a<br>
>> >> >>> >> >> CXXRecordDecl<br>
>> >> >>> >> >> and<br>
>> >> >>> >> >> see if they should instead take a RecordDecl<br>
>> >> >>> >> >> 3) Add unionDecl() as a node matcher (or should we add<br>
>> >> >>> >> >> isUnion()<br>
>> >> >>> >> >> and<br>
>> >> >>> >> >> isStruct() as narrowing matchers?)<br>
>> >> >>> >> >> 4) Add tagDecl() as a node matcher, but not add<br>
>> >> >>> >> >> cxxRecordDecl()<br>
>> >> >>> >> ><br>
>> >> >>> >> ><br>
>> >> >>> >> > Why not add cxxRecordDecl()? I think we need it if we want<br>
>> >> >>> >> > narrowing<br>
>> >> >>> >> > matchers on CXXRecordDecl?<br>
>> >> >>> >><br>
>> >> >>> >> If Richard thinks CXXRecordDecl is an anachronism, I figured we<br>
>> >> >>> >> didn't<br>
>> >> >>> >> want to expose it. Instead, we could make hasMethod (et al)<br>
>> >> >>> >> accept<br>
>> >> >>> >> a<br>
>> >> >>> >> RecordDecl and do the type checking for the caller. Then<br>
>> >> >>> >> recordDecl(hasMethod(...)) continues to compile and work, and<br>
>> >> >>> >> when<br>
>> >> >>> >> hasMethod is given a RecordDecl instead of a CXXRecordDecl, it<br>
>> >> >>> >> simply<br>
>> >> >>> >> matches nothing. But you bring up a good point about the C++ DSL<br>
>> >> >>> >> being<br>
>> >> >>> >> a problem still, I hadn't considered that.<br>
>> >> >>> ><br>
>> >> >>> ><br>
>> >> >>> > First I want Richard to confirm that. I have a very bad memory,<br>
>> >> >>> > so I<br>
>> >> >>> > might<br>
>> >> >>> > as well misremember :)<br>
>> >> >>> ><br>
>> >> >>> >><br>
>> >> >>> >><br>
>> >> >>> >> ~Aaron<br>
>> >> >>> >><br>
>> >> >>> >> ><br>
>> >> >>> >> >><br>
>> >> >>> >> >><br>
>> >> >>> >> >> ~Aaron<br>
>> >> >>> >> >><br>
>> >> >>> >> >> ><br>
>> >> >>> >> >> >><br>
>> >> >>> >> >> >><br>
>> >> >>> >> >> >> Thanks!<br>
>> >> >>> >> >> >><br>
>> >> >>> >> >> >> ~Aaron<br>
>> >> ><br>
>> >> ><br>
</blockquote></div>