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 <kli...@google.com> 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 <kli...@google.com> wrote: >> > On Tue, Sep 8, 2015 at 3:23 PM Aaron Ballman <aa...@aaronballman.com> >> wrote: >> >> >> >> On Tue, Sep 8, 2015 at 9:18 AM, Manuel Klimek <kli...@google.com> >> wrote: >> >> > On Tue, Sep 8, 2015 at 2:23 PM Aaron Ballman <aa...@aaronballman.com >> > >> >> > wrote: >> >> >> >> >> >> 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. >> >> > >> >> > >> >> > 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 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 problem still, I hadn't considered that. >> > >> > >> > First I want Richard to confirm that. I have a very bad memory, so I >> might >> > as well misremember :) >> > >> >> >> >> >> >> ~Aaron >> >> >> >> > >> >> >> >> >> >> >> >> >> ~Aaron >> >> >> >> >> >> > >> >> >> >> >> >> >> >> >> >> >> >> Thanks! >> >> >> >> >> >> >> >> ~Aaron >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
