[PATCH] D158872: [clang][ASTMatchers] Add a few type-related Matchers

Tue Aug 29 06:51:21 PDT 2023

aaron.ballman added reviewers: r4nt, sammccall.
aaron.ballman added a comment.

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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158872/new/

https://reviews.llvm.org/D158872