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? > > ~Aaron > > > > >> > >> > >> Thanks! > >> > >> ~Aaron >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
