aaron.ballman added a comment. In D158872#4625425 <https://reviews.llvm.org/D158872#4625425>, @sammccall wrote:
> In D158872#4624821 <https://reviews.llvm.org/D158872#4624821>, @aaron.ballman > wrote: > >> In D158872#4623193 <https://reviews.llvm.org/D158872#4623193>, @danix800 >> wrote: >> >>> In D158872#4622124 <https://reviews.llvm.org/D158872#4622124>, >>> @aaron.ballman wrote: >>> >>>> Are these matchers going to be used in-tree (by clang-tidy, or something >>>> else)? We typically do not add new AST matches until there's a need for >>>> them because the AST matchers have a pretty big impact on build times of >>>> Clang itself. >>> >>> They are used in `ASTImporter` testcases as shown in >>> https://reviews.llvm.org/D158948. Though this might not be a strong reason >>> to >>> add these matchers and bring on too much bad impact. >>> >>> ASTImporter is more urgent since we still lack support for some of the AST >>> nodes so I considered adding them batchly and started with >>> type-related nodes. I thought that matchers're OK to use in unittests as >>> actually they are used a lot there, but I wasn't aware of the impact >>> on build of Clang. >>> >>> If not acceptable, I'm OK with it. We can still test importing with other >>> ways. Matchers are not mandatory. >>> >>> EDIT: If acceptable, tools like `clang-query` can benefit from these >>> matchers too, easy for >>> debugging I guess. Please let me known whether I should proceed on this >>> revision or not. Thanks for reviewing anyway! >> >> Pulling in a few more folks for extra opinions in case I'm off-base, but I >> think a good approach for right now is to add these matches to the unit test >> file directly in the AST importer patch and to hold off on landing this one. >> Then you still get the test coverage you need in the unit tests, but the >> extra compilation time is limited to just one .cpp file and not everything >> including `ASTMatchers.h`. As we find a need for the matchers, we can lift >> them from the unit test into the public matchers. It's not that there's no >> utility to these (there is!), it's more just the balancing act between >> compile times and utility. > > This is a sad state of affairs :-( Agreed, but it's also not a new state of affairs; we've held this position for as long as we've had AST matchers. (For reference, ASTMatchers.h creates so many symbols we needed to enable /bigobj just to be able to generate an object file that could represent it -- you use that option when you have > 65k addressable sections in an object file!) > Not having "obvious" matchers for some types/properties in the AST is a real > problem inhibiting use of matchers IME. People will use fragile hacks or give > up entirely rather than write custom matchers, and missing base matchers for > certain types is perceived as a bug. > To have someone recognize a gap & contribute matchers, and not accept the > patch because matchers are too slow to compile, is pretty painful. > But the compile time issue is real! Agreed. :-( > A few ideas: > > - could we move these (and possibly other rarely-used matchers) to > ObscureMatchers.h or something? It's definitely ugly, but maybe not as bad as > missing them entirely. > - seems like we should extract an ASTMatchersBase.h or so to separate > "matcher framework" from "all the slow matchers" so headers that expose > matcher-based APIs don't have to transitively expose everything, reducing the > cost of such changes > - (long shot) maybe we can work out which code patterns are particularly > terrible for parse times and change them The problem is the pile of templates that is used to generate the DSL and used for dynamic matchers by clang-query. IIRC, we looked into reducing the overhead and eventually punted on it, some background can be found here: https://reviews.llvm.org/D84038 when we finally decided to enable /bigobj everywhere. I know we did a pass to split ASTMatchers.h up so we now have ASTMatchersInternal.h, so I think we might have done about as much extraction as we can (though it'd be worth seeing if there's more we can do now that we've got access to C++17 in source; I think we were still on C++11 when we last did this). Ideally, I would love to table generate more of the AST matchers from source such as https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/TypeNodes.td and https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/DeclNodes.td so that we automatically get all of the type and declaration matchers (at least the top-level matchers) without any extra maintenance overhead to keep them in sync. But I've always had the impression this idea was dead in the water because of overhead. I suspect moving things around into multiple headers won't help all that much (and it will likely cause confusion as to what header file to put any given AST matcher into) because Clang itself will still need to include all of those headers (and thus will still hit all the compile time and object size problems). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158872/new/ https://reviews.llvm.org/D158872 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
