[PATCH] D138329: [-Wunsafe-buffer-usage] Add a new recursive matcher to replace `forEachDescendant` in unsafe buffer check

Wed Jan 4 18:02:14 PST 2023

NoQ added a comment.

In D138329#4027490 <https://reviews.llvm.org/D138329#4027490>, @thakis wrote:

> The `clang` binary should not depend on `ASTMatchers`.
>
> I see this isn't new in this patch, but `ASTMatchers` should only be used from clang-tidy, not from the compiler itself.

ASTMatchers have been baked into clang binary for at least a few years already, actively used by the static analyzer (some backstory in D25429 <https://reviews.llvm.org/D25429>). This warning is, however, probably the first use in clang proper, so they'll be there even if static analyzer support is turned off through cmake flags.

I'm open to a broader discussion. To me it's natural that there exist "analysis-based warnings" that take advantage of advanced analysis tools, and ASTMatchers is just one such tool, definitely not the most expensive one and not the most hazardous one (CFG and flow-sensitive analysis are arguably much scarier in both regards, and they're actively used in essential warnings such as `-Wuninitialized`).

With respect to this warning, we're also paying a lot of attention to make sure that performance is acceptable for the compiler. This patch is actually an example of our commitment to performance as it flattens the performance of this analysis down to linear time over AST size, something that wasn't previously possible with ASTMatchers.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138329