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

[Artem Dergachev 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>


haoNoQ wrote:

The static analyzer handles this pretty well already. I haven't heard of any problems in this area. I think it makes sense to use the same logic by default in other tools unless you do have specific concerns.

Also please take a note of the test case [check-deserialization.cpp](https://github.com/llvm/llvm-project/blob/main/clang/test/Analysis/check-deserialization.cpp) which demonstrates that the static analyzer not only skips the analysis of system headers - but it also skips deserialization of that code when PCHs/modules are involved. Which might be another source of performance gains for your patch. It might be a good idea to generalize this test case to clang-tidy.

> Note2: out of all the unit tests, only one of them fails:
> readability/identifier-naming-anon-record-fields.cpp

I feel a sudden urge to channel my inner Mr. Heckles.

![mr_heckles](https://github.com/user-attachments/assets/0f00b458-d96d-46c9-9b89-1ecfc1e2f0ac)

It's probably true that most of the existing clang-tidy checks are unaffected by this change. That said, I suspect that your patch alters the underlying contract of clang-tidy checker API quite significantly.

For example, with the current design, the checker is allowed to confirm the presence of a certain declaration in the included system headers before making other decisions. For example, a checker may refuse to emit a fix-it hint that suggests `std::move` if it knows that `std::move` is not defined in the current translation unit. (Or maybe the checker will turn itself off entirely in this situation.) And it is allowed to figure that out by simply scanning the translation unit for a declaration of a function named `move` inside a namespace named `std`. With your implementation, the checker will be unable to rely on the results of such scan, given that the system header will refuse to get scanned.

So I think your patch should be seen as more than a performance optimization; it's a somewhat significant change of contract between the checker developer and the tool's engine.

I see a few conversations about that in the previous discussions of that patch too:
> https://reviews.llvm.org/D150126#4359323
> I love an idea, but i'm afraid that it may impact checks that build some internal database, like misc-confusable-identifiers or misc-unused-using-decls.

Do we have some kind of consensus on this change of direction? I'm personally on board with this either way but I'm not a clang-tidy maintainer.

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