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
