On Fri, Sep 11, 2015 at 10:39 PM Aaron Ballman <aa...@aaronballman.com> wrote:
> 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()? > 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. I think given that we want to keep CXXRecordDecl, the right solution is to have a cxxRecordDecl() matcher. 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. > > ~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
