On Fri, Sep 11, 2015 at 4:30 PM, Richard Smith <rich...@metafoo.co.uk> wrote: > 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).
Okay, so then it sounds like we want recordDecl to *mean* RecordDecl, but we want the traversal and narrowing matchers that currently take a CXXRecordDecl to instead take a RecordDecl and handle the CXX part transparently? This means we would not need to add a cxxRecordDecl() matcher, but could still access CXX-only functionality (like access control, base classes, etc) through recordDecl()? ~Aaron > > 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
