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
