[clang-tools-extra] [clang-tidy] Avoid processing declarations in system headers (PR #128150)

[Balazs Benics via cfe-commits]

Carlos =?utf-8?q?Gálvez?= <carlos.galvez at zenseact.com>,
Carlos =?utf-8?q?Gálvez?= <carlos.galvez at zenseact.com>
Message-ID:
In-Reply-To: <llvm.org/llvm/llvm-project/pull/128150 at github.com>


steakhal wrote:

> > We do this by prepending a new ASTConsumer to the list of consumers: this new consumer sets the traversal scope in the ASTContext, which is later used by the MatchASTConsumer.
> 
> I do not like this approach. Would it be possible to add this feature to the existing `MatchASTConsumer` via `MatchFinderOptions`? Doing this in a single ASTConsumer might have less overhead. But more importantly, all clients of ASTMatchers could benefit from this change this way, it would not be limited to Clang Tidy only.
> 
> > it also skips deserialization of that code when PCHs/modules are involved.
> 
> I agree with @haoNoQ, this could be a huge performance improvement. It is worth checking what happens in this scenario and whether we could avoid deserialisation if it is happening. That being said, I'd also prefer that to be part of the ASTMatchers library instead of a Clang Tidy local solution.

I agree with these statements. It would be awesome if we could generalize the scope reduction to only accept decls spelled in the main file. For instance, if a tool would want to only analyze a specific file, because that is the only file they have control over.

W.r.t. ways forward, I highly discourage implicit slowdowns, for example if a wrong-doing check is enabled that accidentally does some rough AST traversal. It's too tempting already to do an AST traversal from the TU decl - breaking the PCH deserialization test in CSA. I've been there, done that. What I'm saying is that it may seem as an opportunistic performance optimization at first, but then once we get used to it, it will become a silent and serious slowdown regression under specific cases.

https://github.com/llvm/llvm-project/pull/128150