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

Tue Aug 29 12:05:46 PDT 2023

aaron.ballman added subscribers: carlosgalvezp, LegalizeAdulthood.
aaron.ballman added a comment.

In D158872#4626065 <https://reviews.llvm.org/D158872#4626065>, @sammccall wrote:

> I did some testing on my machine (host compiler clang-14), and I'm not sure the specifics justify the level of caution here:

Thank you for doing some research on this!

> ASTMatchers.h parses in 6.5s (with `clang-check`)
> If I comment out the code + internal ASTMatcher includes, then just the deps (mostly AST headers) parses in 4.9s.
> 1.6s for matchers impl is slow but not build-breakingly so. I don't think we should be scared of the marginal parse time increase from this change. (ASTMatchers is not in all that many TUs anyhow - no "core" headers re-export it).

Out of curiosity, are you testing on Windows with MSVC? My understanding on build time performance impacts was that it's debug builds with MSVC that get hit especially hard. (I can test that setup but I could use some help figuring out how you tested compile times so I can do a similar test instead of something totally different from what you tried.)

It would be fantastic if the build time issues were not a problem!

> The bug referenced is about object size. The connection with matchers seems to be that matcher tests had their section limit increased, before this applied to the whole project.
> However this is specifically for tests that test most of the matchers, you only pay for what you use.
> I verified that a cpp file that includes `ASTMatchers.h` and does nothing else generates a trivial-sized object file (<1kb). So again, I don't think we need to fear this change: if it's used in many places, it's justified, and if it's not used, it's not expensive.
>
> (If compilation of files like `NodeMatchersTest.cpp` are significant build bottlenecks, we'd need to address that, but *those* would be fine to split up in unprincipled ways, so I'm not too concerned)

FWIW, we had to increase the limits on various files until we finally broke down and did it project-wide. So it wasn't just limited to testing files having the issue; I think the big problem was templated symbols from instantiations. But as you say, if it's pay-for-what-you-use, that's pretty reasonable.

CC @PiotrZSL @LegalizeAdulthood @carlosgalvezp as folks who work on clang-tidy, which uses AST matchers all over the place. They may also have some good insights or preferences here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158872