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

[Carlos Galvez 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>


carlosgalvezp wrote:

Thank you all for the valuable input! There's a lot to process :) I will summarize the feedback, hopefully I got it right:

- There are more things that can be optimized (clang-tidy's HeaderFilter, PCH, modules).
- clang-tidy now receives less info, thus it places additional limitations on what checks can do (possibly increasing FN rate).
- It would be good to have this logic centralized in `MatchASTConsumer` instead, so all tools that use it benefit from it.

My intention is to take an iterative development approach, providing immediate value to users (who have been suffering this issue since a long time) in small increments, with possibility for easy revert if things don't work out well. 

I'm of the opinion that "perfect is the enemy of the good". For that reason, it's not the scope of this patch to enable further optimizations, nor to enable this for other tools (which would require further acquaintance, investigation, implementation, testing, review, and consensus from owners).  All of that can be done in follow-up patches, and I'm happy to help out with that given support from tool owners.

There's an RFC around this topic [here](https://discourse.llvm.org/t/rfc-exclude-issues-from-system-headers-as-early-as-posible/68483). I believe there's consensus on skipping system headers, but the implementation is a bit more open for discussion. This is the only concrete implementation I've seen available, however. 

Regarding concrete open questions:

- Moving the implementation to `MatchASTConsumer`. 
  - It should be doable, still defaulting to **not** skip system headers, and let clang-tidy skip them. A follow-up patch could change that default so all tools behave in the same way. 
  
-  "may impact checks that build some internal database, like misc-confusable-identifiers or misc-unused-using-decls"
    - I have not seen any failing tests for these checks.

- "Is this a bug that we could fix?"
  - I did try having a look at it but could not find a solution. Perhaps the author of that test (@Lancern ) has some ideas?

- **Let the checkers force the scanning of system headers**
  - Yes! This would be extremely good. However, given my limited knowledge, I don't know how to achieve this in practice. Possibly it requires a major refactoring. Once again, I'm open for suggestions!

- W.r.t. ways forward, I highly discourage implicit slowdowns
  - If I understand correctly, you mean that this patch could lead to people manually traversing the AST instead of using the MatchFinder, thus making specific checks very slow. Correct? I'm not quite sure how to act upon this, do you have a specific suggestion? I would hope deviating from the "standard pattern" would be caught during code review. Otherwise, this would come out as a bug report fairly soon, as it's easy to profile. So I don't see it as high risk. 










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