On Mon, Sep 14, 2015 at 1:37 PM, Manuel Klimek <kli...@google.com> wrote: > On Mon, Sep 14, 2015 at 10:30 AM Daniel Jasper <djas...@google.com> wrote: >> >> On Mon, Sep 14, 2015 at 7:24 PM, Manuel Klimek <kli...@google.com> wrote: >>> >>> On Mon, Sep 14, 2015 at 10:21 AM Daniel Jasper <djas...@google.com> >>> wrote: >>>> >>>> So, back in the day when we implemented the matchers, we decided on >>>> actually wanting to remove all the CXX... AST nodes (there are more of >>>> them). >>> >>> >>> Note that Richard has paddled back on this and now says the CXX... AST >>> nodes are the right thing. >>> >>>> >>>> I don't know how this would work as recordDecl already exists. But I'd >>>> be somewhat hesitant to introduce a cxxRecordDecl matcher if there is still >>>> a chance that we want to move away from the CXX prefix. >>> >>> >>> See above. >> >> >> So do we change all the others at the same time, e.g. create a >> cxxConstructExpr() matcher? There it make less sense as there is no >> ConstructExpr, just CXXConstructExpr. >> >>>> Also note that AST matchers are used massively in the wild by now and I >>>> would be very hesitant to make a change breaking backwards compatibility. >>>> 85 >>>> failures in the clang repositories themselves sounds scary to me. Not >>>> saying >>>> we shouldn't do it at all. But we should be very clear on where things >>>> should be in the long run. >>> >>> >>> Aaron has clarified that that's only 14 outside the AST matcher tests >>> themselves. >> >> >> Sure, still a lot IMO :(. But ok, maybe there is just no other way.
There was one additional file with two instances that was sneakily hiding in my output, so the total count is 16. > I think the trade-off is expected confusion this causes in the future, and > thus people wasting time, vs. the on-time cost of migrating. FWIW (as someone who has been writing a lot of matcher code lately for C and C++), I've never been confused by ctorInitializer() instead of cxxCtorInitializer() (et al), but I spent an embarrassing amount of time learning that recordDecl() meant CXXRecordDecl instead of RecordDecl. Have we ever documented the AST matchers as a stable API? I guess I've always assumed they were like the rest of LLVM's C++ APIs -- you can use them, but sometimes you need to migrate your code because we make API changes. I think that for the cases where there is no corresponding unprefixed version, the chances of getting confused are low. However, I do slightly worry that unprefixed versions may conflict with future language features in C, but not to the point I would lose sleep. If we're talking about breaking people's code to be consistent with the AST node nomenclature, we *could* break it consistently everywhere (a lot of pain, but only one-time pain in theory). However, does that then imply we cannot change AST node names because it would break this mapping? In the case of RecordDecl/CXXRecordDecl, there's a definite pain point for matcher usability. In the other cases, I think the return we receive on breaking people's code is considerably less valuable, but I've not done an exhaustive search of name mappings. If people think that's of value, I could do that (it may also give us a good idea of where we have AST nodes that aren't currently matchable). ~Aaron > Richard, do we plan to remove the CXX prefix from nodes that do not have > non-CXX versions? > > >> >> >>>> >>>> >>>> On Mon, Sep 14, 2015 at 7:03 PM, Aaron Ballman <aa...@aaronballman.com> >>>> wrote: >>>>> >>>>> On Mon, Sep 14, 2015 at 11:49 AM, Manuel Klimek <kli...@google.com> >>>>> wrote: >>>>> > >>>>> > >>>>> > On Mon, Sep 14, 2015, 8:40 AM Aaron Ballman <aa...@aaronballman.com> >>>>> > wrote: >>>>> >> >>>>> >> On Sat, Sep 12, 2015 at 11:06 PM, Manuel Klimek <kli...@google.com> >>>>> >> wrote: >>>>> >> > >>>>> >> > >>>>> >> > 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 :) >>>>> >> >>>>> >> I've attached a patch that implements #2, but it comes with ~85 >>>>> >> errors >>>>> >> from C++ matchers that use recordDecl to mean cxxRecordDecl. >>>>> >> >>>>> >> http://pastebin.com/bxkRcqBV >>>>> >> >>>>> >> If this is an acceptable failure rate, I can also update the failing >>>>> >> matchers to use cxxRecordDecl instead of recordDecl where >>>>> >> applicable. >>>>> >> Doing some spot-checking of the failing code, the failures are ones >>>>> >> we >>>>> >> anticipated, such as: >>>>> >> >>>>> >> constructorDecl(ofClass(recordDecl( >>>>> >> hasDeclaration(recordDecl(hasMethod(hasName("begin")), >>>>> >> hasMethod(hasName("end")))) >>>>> >> etc >>>>> > >>>>> > >>>>> > +Daniel for another opinion. I think this is fine, but I'd prefer >>>>> > not to >>>>> > end up in a meme :) >>>>> >>>>> FWIW, the vast majority of the errors are in ASTMatchersTests. There >>>>> were only a few in actual real-world uses (71 in tests, 14 in real >>>>> code). >>>>> >>>>> ~Aaron >>>>> >>>>> > >>>>> >> ~Aaron >>>>> >> >>>>> >> > >>>>> >> >> >>>>> >> >> ~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
