On Tue, Sep 8, 2015 at 5:40 AM, Manuel Klimek <kli...@google.com> wrote: > Yea, we had that discussion a few times, and I can never remember why we > ended up in the state we're in. > We definitely had a time where we switched to just using the exact same name > as the node's class name for the matchers. > I *think* we didn't do it for cxxRecordDecl, because Richard said that's a > relic we should get rid of anyway, but I'm not sure.
FWIW, I think the state we're in is the worst of all worlds. It's not intuitive that recordDecl() doesn't match a struct in C mode, and as it stands, there is no way to match a struct or union declaration in C at all. > > 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. 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
