<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Fri, Sep 11, 2015 at 10:39 PM 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 Fri, Sep 11, 2015 at 4:30 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
> I don't think CXXRecordDecl is an anachronism, so much as an implementation<br>
> detail; it makes sense to use a smaller class when in C mode, as we don't<br>
> need most of the features and complexity that CXXRecordDecl brings with it.<br>
> But... as a user of clang matchers, I don't think I'd want to care about the<br>
> difference, and it'd be more convenient if I could nest (say) a hasMethod<br>
> matcher within a recordDecl matcher, since it's completely obvious what 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 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></blockquote><div><br></div><div>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.</div><div>I think given that we want to keep CXXRecordDecl, the right solution is to have a cxxRecordDecl() matcher.</div><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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>> wrote:<br>
>><br>
>> Richard! We need an informed opinion :D<br>
>><br>
>> On Fri, Sep 11, 2015 at 3:07 PM Aaron Ballman <<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>> wrote:<br>
>>> > On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman <<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 <<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 <<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 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 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 Richard 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. It's<br>
>>> >> >> not<br>
>>> >> >> intuitive that recordDecl() doesn't match a struct in C mode, and<br>
>>> >> >> as<br>
>>> >> >> it stands, there is no way to match a struct or union declaration<br>
>>> >> >> in C<br>
>>> >> >> at all.<br>
>>> >> ><br>
>>> >> ><br>
>>> >> > Agreed. Best intentions. Worst possible outcome. That's 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 match<br>
>>> >> >> >> RecordDecl objects; instead, it matches CXXRecordDecl 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 declaration<br>
>>> >> >> >> in C<br>
>>> >> >> >> or ObjC. However, given how prevalent recordDecl()'s use is in<br>
>>> >> >> >> the<br>
>>> >> >> >> wild (I'm guessing), changing it at this point would be a Bad<br>
>>> >> >> >> Thing.<br>
>>> >> >> >><br>
>>> >> >> >> For people trying to write AST matchers for languages like C 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 functionality<br>
>>> >> >> >> exposed<br>
>>> >> >> >> by recordDecl(), it more clearly matches the intention of which<br>
>>> >> >> >> AST<br>
>>> >> >> >> node it corresponds to.<br>
>>> >> >> >><br>
>>> >> >> >> Finally, I would like to undocument recordDecl() and change 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 acceptable, 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 few<br>
>>> >> >> > tools<br>
>>> >> >> > will<br>
>>> >> >> > actually start doing wrong things because of it. I'd like more<br>
>>> >> >> > opinions<br>
>>> >> >> > first, though :)<br>
>>> >> >><br>
>>> >> >> I was giving this more thought over the long weekend, and I think<br>
>>> >> >> you<br>
>>> >> >> may be right. I think changing recordDecl() to mean RecordDecl 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 we<br>
>>> >> >> need<br>
>>> >> >> to fix up at the same time. When I tried to think of AST matchers<br>
>>> >> >> that<br>
>>> >> >> mean CXXRecordDecl but *not* RecordDecl, they were horribly<br>
>>> >> >> contrived<br>
>>> >> >> because you usually are matching on additional selection criteria<br>
>>> >> >> that<br>
>>> >> >> is specific to C++ (such as hasMethod() or isDerivedFrom()) which<br>
>>> >> >> would cause the match to continue to fail, as expected. Code that<br>
>>> >> >> uses<br>
>>> >> >> recordDecl() to mean RecordDecl will suddenly start to match 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 would be<br>
>>> >> >> where you wrote recordDecl(), meant CXXRecordDecl, had no further<br>
>>> >> >> narrowing or traversal matchers, and were compiling in C mode; 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 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 CXXRecordDecl<br>
>>> >> >> and<br>
>>> >> >> see if they should instead take a RecordDecl<br>
>>> >> >> 3) Add unionDecl() as a node matcher (or should we add isUnion()<br>
>>> >> >> and<br>
>>> >> >> isStruct() as narrowing matchers?)<br>
>>> >> >> 4) Add tagDecl() as a node matcher, but not add cxxRecordDecl()<br>
>>> >> ><br>
>>> >> ><br>
>>> >> > Why not add cxxRecordDecl()? I think we need it if we want narrowing<br>
>>> >> > matchers on CXXRecordDecl?<br>
>>> >><br>
>>> >> If Richard thinks CXXRecordDecl is an anachronism, I figured we didn't<br>
>>> >> want to expose it. Instead, we could make hasMethod (et al) accept a<br>
>>> >> RecordDecl and do the type checking for the caller. Then<br>
>>> >> recordDecl(hasMethod(...)) continues to compile and work, and when<br>
>>> >> hasMethod is given a RecordDecl instead of a CXXRecordDecl, it simply<br>
>>> >> matches nothing. But you bring up a good point about the C++ DSL 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, 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></div>