sammccall added a comment. 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 :-( 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! 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 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
