On Sat, Sep 12, 2015, 9:25 PM Aaron Ballman <aa...@aaronballman.com> wrote:
> On Sat, Sep 12, 2015 at 8:22 AM, Manuel Klimek <kli...@google.com> wrote: > > > > > > 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. > > Personally, I think this makes the most sense, at least to me. The > recommendation I've always heard (and given) is to use -ast-dump and > write matchers from there. (Consequently, the more I work with type > traversal matchers, the more I wish we had -ast-dump-types to give > even *more* information for writing matchers.) > > But the question still remains, what do we do with recordDecl? Right > now, it means CXXRecordDecl instead of RecordDecl. If we change it to > mean RecordDecl instead, there's a chance we'll break existing, > reasonable code. Are we okay with that risk? If we're at least > conceptually okay with it, I could make the change locally and see > just how much of our own code breaks, and report back. But if that > turns out to be problematic, do we want to deprecate recordDecl and > replace it with structDecl as our fallback position? Or is there a > better solution? > > Basically, I see a few ways to solve this (and there may be other ways > I'm not thinking about yet): > > 1) Undocument/deprecate recordDecl, add structDecl, unionDecl, and > cxxRecordDecl. This does not match the AST because we have no > StructDecl or UnionDecl types. The more I think about this option, the > less I like it. It's easy to implement, but seems hard to relate to > the AST. > 2) Make recordDecl match RecordDecl, don't touch other matchers. Add > way to distinguish unions from structs (e.g., isUnion(), isStruct()), > add cxxRecordDecl. This matches the AST most closely, but may break > code. I think that I prefer this approach, but it depends heavily on > what "may break code" looks like in practice. > 3) Make recordDecl match RecordDecl, fix other matchers that currently > take CXXRecordDecl to instead take RecordDecl and handle sensibly when > possible. Add a way to distinguish unions from structs, add > cxxRecordDecl. This doesn't match the AST because we will have > matchers taking a RecordDecl when the AST would require a > CXXRecordDecl, but is likely to break less code. > That sums it up. My preferences are 2, 3, 1 in that order :) > ~Aaron > > > > 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
