I've commit in r247885 and r247886. I will add something to the release notes, and watch the bots to see if any tests got missed (since I did my development on Windows).
Thank you! ~Aaron On Wed, Sep 16, 2015 at 7:49 PM, Manuel Klimek <kli...@google.com> wrote: > LG, ship it. > > On Wed, Sep 16, 2015 at 2:03 PM Aaron Ballman <aa...@aaronballman.com> > wrote: >> >> Attached is an updated patch for clang-tools-extra that does not have >> my in-progress, not-related-to-any-of-this code in it. ;-) >> >> ~Aaron >> >> On Wed, Sep 16, 2015 at 4:04 PM, Aaron Ballman <aa...@aaronballman.com> >> wrote: >> > Quick ping. I know this is a fairly gigantic patch, but I'm hoping for >> > a relatively quick review turnaround because of potential merge >> > conflicts with people doing a fair amount of work on clang-tidy >> > lately. Everything should be pretty straight-forward (it's all just >> > renames, no semantic changes intended aside from >> > recordDecl/cxxRecordDecl and the narrowing matchers. >> > >> > ~Aaron >> > >> > On Tue, Sep 15, 2015 at 1:32 PM, Aaron Ballman <aa...@aaronballman.com> >> > wrote: >> >> Here are the complete patches to solve the issues we've discussed: >> >> >> >> 1) splits recordDecl into recordDecl and cxxRecordDecl >> >> 2) adds isStruct, isUnion, isClass to identify what kind of >> >> recordDecl() you may be looking at >> >> 3) prefixes all of the node matchers with cxx that should have it >> >> 4) fixes a similar issue with CUDAKernelCallExpr (the prefix needs to >> >> be cuda instead of CUDA to distinguish the matcher name from the type >> >> name). >> >> 5) updates all of the documentation and code that broke. >> >> >> >> One patch is for changes to clang, the other is for changes to >> >> clang-tools-extra. >> >> >> >> ~Aaron >> >> >> >> On Mon, Sep 14, 2015 at 5:49 PM, Aaron Ballman <aa...@aaronballman.com> >> >> wrote: >> >>> On Mon, Sep 14, 2015 at 5:47 PM, Daniel Jasper <djas...@google.com> >> >>> wrote: >> >>>> Btw, I think generating them, potentially into several different >> >>>> headers to >> >>>> work around the compile time issue isn't such a bad idea. >> >>> >> >>> I'm not going to start with this approach, but think it may be worth >> >>> exploring at some point. ;-) >> >>> >> >>> ~Aaron >> >>> >> >>>> >> >>>> On Mon, Sep 14, 2015 at 11:45 PM, Manuel Klimek <kli...@google.com> >> >>>> wrote: >> >>>>> >> >>>>> Feel free to rename the AST nodes :) >> >>>>> >> >>>>> >> >>>>> On Mon, Sep 14, 2015, 2:44 PM Daniel Jasper <djas...@google.com> >> >>>>> wrote: >> >>>>>> >> >>>>>> Ok. I am happy with this then. >> >>>>>> >> >>>>>> (Just personally grumpy having to write >> >>>>>> cxxRecordDecl(has(cxxConstructorDecl(..))) in the future ;-) ). >> >>>>>> >> >>>>>> On Mon, Sep 14, 2015 at 11:41 PM, Manuel Klimek <kli...@google.com> >> >>>>>> wrote: >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> On Mon, Sep 14, 2015 at 2:29 PM Aaron Ballman >> >>>>>>> <aa...@aaronballman.com> >> >>>>>>> wrote: >> >>>>>>>> >> >>>>>>>> On Mon, Sep 14, 2015 at 4:38 PM, Manuel Klimek >> >>>>>>>> <kli...@google.com> >> >>>>>>>> wrote: >> >>>>>>>> > >> >>>>>>>> > >> >>>>>>>> > On Mon, Sep 14, 2015 at 12:26 PM Aaron Ballman >> >>>>>>>> > <aa...@aaronballman.com> >> >>>>>>>> > wrote: >> >>>>>>>> >> >> >>>>>>>> >> On Mon, Sep 14, 2015 at 2:45 PM, Daniel Jasper >> >>>>>>>> >> <djas...@google.com> >> >>>>>>>> >> wrote: >> >>>>>>>> >> > By this point, I see that change might be profitable >> >>>>>>>> >> > overall. >> >>>>>>>> >> > However, >> >>>>>>>> >> > lets >> >>>>>>>> >> > completely map this out. Changing just cxxRecordDecl() can >> >>>>>>>> >> > actually >> >>>>>>>> >> > increase >> >>>>>>>> >> > confusion in other areas. Right now, not a single AST >> >>>>>>>> >> > matcher has >> >>>>>>>> >> > the >> >>>>>>>> >> > cxx >> >>>>>>>> >> > prefix (although a total of 28 stand for the corresponding >> >>>>>>>> >> > CXX.. >> >>>>>>>> >> > AST >> >>>>>>>> >> > node). >> >>>>>>>> >> > This is consistent and people knowing this will never try to >> >>>>>>>> >> > write >> >>>>>>>> >> > cxxConstructExpr(). As soon as people have used >> >>>>>>>> >> > cxxRecordDecl(), >> >>>>>>>> >> > the >> >>>>>>>> >> > chance >> >>>>>>>> >> > of them trying cxxConstructExpr() increases. You have spent >> >>>>>>>> >> > a long >> >>>>>>>> >> > time >> >>>>>>>> >> > figuring out that recordDecl means cxxRecordDecl(), which is >> >>>>>>>> >> > one >> >>>>>>>> >> > datapoint, >> >>>>>>>> >> > but I am not aware of anyone else having this specific >> >>>>>>>> >> > issue. And >> >>>>>>>> >> > we >> >>>>>>>> >> > could >> >>>>>>>> >> > make this less bad with better documentation, I think. >> >>>>>>>> >> > >> >>>>>>>> >> > So, for me, the questions are: >> >>>>>>>> >> > 1) Do we want/need this change? >> >>>>>>>> >> >> >>>>>>>> >> We definitely need *a* change because there currently is no >> >>>>>>>> >> way to >> >>>>>>>> >> match a C struct or union when compiling in C mode. I >> >>>>>>>> >> discovered >> >>>>>>>> >> this >> >>>>>>>> >> because I was trying to write a new checker for clang-tidy >> >>>>>>>> >> that >> >>>>>>>> >> focuses on C code and it would fail to match when compiling in >> >>>>>>>> >> C >> >>>>>>>> >> mode. >> >>>>>>>> >> Whether we decide to go with cxxRecordDecl vs recordDecl vs >> >>>>>>>> >> structDecl >> >>>>>>>> >> (etc) is less important to me than the ability to write >> >>>>>>>> >> clang-tidy >> >>>>>>>> >> checks for C code. >> >>>>>>>> >> >> >>>>>>>> >> > 2) Do we want to be consistent and change the other 27 >> >>>>>>>> >> > matchers as >> >>>>>>>> >> > well? >> >>>>>>>> >> >> >>>>>>>> >> I'm on the fence about this for all the reasons you point out. >> >>>>>>>> >> >> >>>>>>>> >> > A fundamental question is whether we want AST matchers to >> >>>>>>>> >> > match >> >>>>>>>> >> > AST >> >>>>>>>> >> > nodes >> >>>>>>>> >> > 1:1 or whether they should be an abstraction from some >> >>>>>>>> >> > implementation >> >>>>>>>> >> > details of the AST. >> >>>>>>>> >> >> >>>>>>>> >> I absolutely agree that this is a fundamental question. I >> >>>>>>>> >> think a >> >>>>>>>> >> higher priority fundamental question that goes along with it >> >>>>>>>> >> is: are >> >>>>>>>> >> we okay with breaking a lot of user code (are these meant to >> >>>>>>>> >> be >> >>>>>>>> >> stable >> >>>>>>>> >> APIs like the LLVM C APIs)? If we want these APIs to be >> >>>>>>>> >> stable, that >> >>>>>>>> >> changes the answer of what kind of mapping we can have. >> >>>>>>>> > >> >>>>>>>> > >> >>>>>>>> > I think the AST matchers are so closely coupled to the AST that >> >>>>>>>> > it >> >>>>>>>> > trying to >> >>>>>>>> > be more stable than the AST doesn't help. Basically all uses of >> >>>>>>>> > AST >> >>>>>>>> > matchers >> >>>>>>>> > do something with the AST nodes afterwards, which will break >> >>>>>>>> > anyway. >> >>>>>>>> >> >>>>>>>> I can get behind that logic. So we're okay with breaking their >> >>>>>>>> code >> >>>>>>>> because there's no way around it -- it's tied to the AST, so >> >>>>>>>> users >> >>>>>>>> cannot rely on the AST APIs remaining the same from release to >> >>>>>>>> release >> >>>>>>>> anyway. >> >>>>>>> >> >>>>>>> >> >>>>>>> We might even *want* the code to break, as the use of the AST node >> >>>>>>> might >> >>>>>>> now be incorrect on a semantic level. >> >>>>>>> >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> > >> >>>>>>>> >> >> >>>>>>>> >> > And this is not an easy question to answer. There are >> >>>>>>>> >> > many places where we don't follow a strict 1:1 mapping. >> >>>>>>>> >> > Mostly >> >>>>>>>> >> > node >> >>>>>>>> >> > matchers, but also in traversal matchers, e.g. >> >>>>>>>> >> > isDerivedFrom(). >> >>>>>>>> >> > >> >>>>>>>> >> > Personally, I'd really hate to have the cxx Prefix >> >>>>>>>> >> > everywhere, but >> >>>>>>>> >> > that's >> >>>>>>>> >> > just my personal opinion. I would even prefer matchers like >> >>>>>>>> >> > record() and >> >>>>>>>> >> > method(), but I think somebody convinced me that that would >> >>>>>>>> >> > be a >> >>>>>>>> >> > very >> >>>>>>>> >> > bad >> >>>>>>>> >> > idea ;-). >> >>>>>>>> >> >> >>>>>>>> >> My personal opinion is that (1) breaking code is fine, but we >> >>>>>>>> >> should >> >>>>>>>> >> avoid doing it without very clear benefit, and (2) the mapping >> >>>>>>>> >> between >> >>>>>>>> >> AST node identifiers and AST matcher identifiers needs to be >> >>>>>>>> >> incredibly obvious, but perhaps not slavishly 1:1. If we >> >>>>>>>> >> instead >> >>>>>>>> >> decide we want a 1:1 mapping, then I think we need to start >> >>>>>>>> >> seriously >> >>>>>>>> >> considering auto-generating the AST node (and type) matchers >> >>>>>>>> >> from >> >>>>>>>> >> tablegen so that the AST nodes *cannot* get out of sync with >> >>>>>>>> >> the AST >> >>>>>>>> >> matchers, otherwise we'll be right back here again in a few >> >>>>>>>> >> years >> >>>>>>>> >> when >> >>>>>>>> >> we modify the name of an AST node. >> >>>>>>>> > >> >>>>>>>> > >> >>>>>>>> > I do think we want to auto-generate the matchers, but I don't >> >>>>>>>> > think >> >>>>>>>> > tablegen >> >>>>>>>> > is the right approach (I think an ast-matcher based tool is ;) >> >>>>>>>> > That said, auto-generating all the matchers is a) a lot of >> >>>>>>>> > effort and >> >>>>>>>> > b) the >> >>>>>>>> > code-size and compile time of matchers already matters, so it's >> >>>>>>>> > unclear >> >>>>>>>> > which ones we would want to generate, especially for traversal >> >>>>>>>> > matchers :( >> >>>>>>>> >> >>>>>>>> Oh, that's an excellent point (I'm talking about (b), I already >> >>>>>>>> knew >> >>>>>>>> (a) was a lot of work). Thank you for pointing that out! >> >>>>>>>> >> >>>>>>>> > >> >>>>>>>> >> >> >>>>>>>> >> My definition of "incredibly obvious" is: if the AST has a >> >>>>>>>> >> prefixed >> >>>>>>>> >> and unprefixed version, or two different prefixes, we should >> >>>>>>>> >> mimic >> >>>>>>>> >> that directly with the matchers. Otherwise, existing AST >> >>>>>>>> >> matchers >> >>>>>>>> >> without prefix shenanigans can remain as they are, and new AST >> >>>>>>>> >> matchers should prefix as-required. If we decide we're okay >> >>>>>>>> >> breaking >> >>>>>>>> >> code, then I don't see a problem with changing >> >>>>>>>> >> ctorInitializer() >> >>>>>>>> >> into >> >>>>>>>> >> cxxCtorInitializer() when C adds constructors. ;-) >> >>>>>>>> > >> >>>>>>>> > >> >>>>>>>> > I think the main things is cost for developers who try to write >> >>>>>>>> > matchers and >> >>>>>>>> > work from the -ast-dump. Figuring out that there *is* a matcher >> >>>>>>>> > with >> >>>>>>>> > an >> >>>>>>>> > unprefixed node can take a while. >> >>>>>>>> >> >>>>>>>> Hmm, yes, but "take a while" should be relatively short, I would >> >>>>>>>> think. In that use-case, the user does an -ast-dump, sees >> >>>>>>>> "CXXFrobbleGnasher", they go to the AST matcher reference and >> >>>>>>>> they >> >>>>>>>> search for "CXXFrobberGnasher." The first hit won't be >> >>>>>>>> cxxFrobbleGnasher, but the entry for frobbleGnasher (which is >> >>>>>>>> still >> >>>>>>>> the first hit when searching from the top of the document due to >> >>>>>>>> the >> >>>>>>>> way we position node matchers) will have a parameter of >> >>>>>>>> CXXFrobbleGnasher, so they will find still get to the right >> >>>>>>>> matcher on >> >>>>>>>> the first hit. If someone doesn't read the documentation at all, >> >>>>>>>> they're going to try cxxFrobbleGnasher() and get a compile >> >>>>>>>> error/no >> >>>>>>>> known matcher. Then they'll go look at ASTMatchers.h and figure >> >>>>>>>> out >> >>>>>>>> it's called frobbleGnasher by searching there instead of the >> >>>>>>>> documentation. >> >>>>>>> >> >>>>>>> >> >>>>>>> The problem is that I've learned that sometimes people try to make >> >>>>>>> things work in ways that I couldn't even imagine, and they lose >> >>>>>>> more time >> >>>>>>> than I could ever imagine them using :) Also, I agree the time is >> >>>>>>> probably >> >>>>>>> on average not that large, but we pay it over a long time in the >> >>>>>>> future, and >> >>>>>>> it tends to add up. >> >>>>>>> >> >>>>>>>> >> >>>>>>>> That's compared to having the matcher name always be the same as >> >>>>>>>> the >> >>>>>>>> AST node, where the user writes cxxFrobbleGnasher and it just >> >>>>>>>> works, >> >>>>>>>> which is definitely a mark in favor of making everything >> >>>>>>>> consistent. I >> >>>>>>>> just don't think the current approach is too onerous in the case >> >>>>>>>> where >> >>>>>>>> the matcher is at least *provided* for the user with a relatively >> >>>>>>>> sane >> >>>>>>>> name. >> >>>>>>>> >> >>>>>>>> >> I should be clear, I'm not opposed to just having a 1:1 >> >>>>>>>> >> mapping. I'm >> >>>>>>>> >> just not certain the benefits of being strict about that >> >>>>>>>> >> outweigh >> >>>>>>>> >> the >> >>>>>>>> >> costs to broken code. cxxCtorInitializer will break someone's >> >>>>>>>> >> code, >> >>>>>>>> >> but I don't think it adds any clarity to their code, so I >> >>>>>>>> >> don't see >> >>>>>>>> >> the benefit of forcing the change. >> >>>>>>>> > >> >>>>>>>> > Well, I think there's the cost of broken code *once* now, vs. >> >>>>>>>> > the >> >>>>>>>> > (smaller) >> >>>>>>>> > cost for users in all future. >> >>>>>>>> > I'm still strongly in favor of breaking now, and having a >> >>>>>>>> > simpler >> >>>>>>>> > model >> >>>>>>>> > going forward. >> >>>>>>>> >> >>>>>>>> I'm definitely in favor of breaking now in the case of RecordDecl >> >>>>>>>> vs >> >>>>>>>> CXXRecordDecl. I think having recordDecl match CXXRecordDecl is a >> >>>>>>>> bug >> >>>>>>>> given that there's no way to match a RecordDecl. >> >>>>>>>> >> >>>>>>>> I would also be totally in favor of being consistent if we were >> >>>>>>>> starting from scratch. I'm very, very weakly opposed to breaking >> >>>>>>>> more >> >>>>>>>> user's code than we have to in order to get usable matchers >> >>>>>>>> because it >> >>>>>>>> seems gratuitous. Breaking code to get something that works seems >> >>>>>>>> reasonable. Breaking code that already works just to change the >> >>>>>>>> name >> >>>>>>>> for consistency elsewhere, I'm a bit less keen on. But the fact >> >>>>>>>> that >> >>>>>>>> we already can break user's code at-will because of the reliance >> >>>>>>>> on >> >>>>>>>> the AST nodes makes me think it may be the right approach for the >> >>>>>>>> best >> >>>>>>>> API, since that's what I would want if we were starting from >> >>>>>>>> scratch. >> >>>>>>>> >> >>>>>>>> Okay, I'm convinced. I think we should rename the type and node >> >>>>>>>> matchers (not traversal and narrowing matchers) to match the AST >> >>>>>>>> node >> >>>>>>>> names in all cases. We can document the breakage in the release >> >>>>>>>> notes, >> >>>>>>>> but (hopefully) only have to do this dance one time instead of >> >>>>>>>> spreading the pain out as it happens to eventually get to the >> >>>>>>>> same >> >>>>>>>> place anyway. >> >>>>>>> >> >>>>>>> >> >>>>>>> Yea, people who want more stability do use releases anyway. >> >>>>>>> >> >>>>>>>> >> >>>>>>>> Daniel, is this something you would be okay with? (I'm assuming >> >>>>>>>> Richard finds it acceptable based on previous comments from >> >>>>>>>> Manuel, >> >>>>>>>> but Richard, feel free to chime in.) >> >>>>>>> >> >>>>>>> >> >>>>>>> Offline conversation with Richard says that he is convinced. >> >>>>>>> >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> ~Aaron >> >>>>>> >> >>>>>> >> >>>> _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
