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
