<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Sep 8, 2015 at 2:23 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 Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:<br>
> Yea, we had that discussion a few times, and I can never remember why we<br>
> ended up in the state we're in.<br>
> We definitely had a time where we switched to just using the exact same name<br>
> as the node's class name for the matchers.<br>
> I *think* we didn't do it for cxxRecordDecl, because Richard said that's 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 not<br>
intuitive that recordDecl() doesn't match a struct in C mode, and as<br>
it stands, there is no way to match a struct or union declaration in C<br>
at all.<br></blockquote><div><br></div><div>Agreed. Best intentions. Worst possible outcome. That's software development :)</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
><br>
> On Fri, Sep 4, 2015 at 8:32 PM Aaron Ballman <<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>> wrote:<br>
>><br>
>> It turns out that the recordDecl() AST matcher doesn't match<br>
>> RecordDecl objects; instead, it matches CXXRecordDecl objects. This<br>
>> is... unfortunate... as it makes writing AST matchers more complicated<br>
>> because of having to translate between recordDecl()/CXXRecordDecl. It<br>
>> also makes it impossible to match a struct or union declaration in C<br>
>> or ObjC. However, given how prevalent recordDecl()'s use is in the<br>
>> wild (I'm guessing), changing it at this point would be a Bad Thing.<br>
>><br>
>> For people trying to write AST matchers for languages like C or 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 matcher.<br>
>><br>
>> Additionally, I would like to add cxxRecordDecl() to match<br>
>> CXXRecordDecl objects. While it duplicates the functionality exposed<br>
>> by recordDecl(), it more clearly matches the intention of which 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 deprecate<br>
>> recordDecl() more officially.<br>
>><br>
>> I'm open to other ideas if there are better ways to move forward. If<br>
>> you think changing the meaning of recordDecl() is acceptable, I can<br>
>> also go that route (though I would still propose adding unionDecl()<br>
>> and cxxRecordDecl() in that case).<br>
><br>
><br>
> I think changing recordDecl is acceptable. I believe very few tools will<br>
> actually start doing wrong things because of it. I'd like more opinions<br>
> first, though :)<br>
<br>
I was giving this more thought over the long weekend, and I think 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 approach<br>
to the change and see which narrowing and traversal matchers we need<br>
to fix up at the same time. When I tried to think of AST matchers that<br>
mean CXXRecordDecl but *not* RecordDecl, they were horribly contrived<br>
because you usually are matching on additional selection criteria 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 uses<br>
recordDecl() to mean RecordDecl will suddenly start to match in more<br>
cases, but that's likely to be a bug fix more than a breaking 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></blockquote><div><br></div><div>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).</div><div>I don't think that's super bad though.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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 and<br>
see if they should instead take a RecordDecl<br>
3) Add unionDecl() as a node matcher (or should we add isUnion() and<br>
isStruct() as narrowing matchers?)<br>
4) Add tagDecl() as a node matcher, but not add cxxRecordDecl()<br></blockquote><div><br></div><div>Why not add cxxRecordDecl()? I think we need it if we want narrowing matchers on CXXRecordDecl?</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>
>><br>
>><br>
>> Thanks!<br>
>><br>
>> ~Aaron<br>
</blockquote></div></div>