[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:

> This does not sound controversial or hard.

It is already controversial for clang-tidy, surely it will be even more controversial if we involve many other tools?

Besides, this request involves a much larger scope (in terms of modified behaviors, not lines of code) than intended. The scope of this patch is only to improve clang-tidy. Everything can always be improved, but it does not need to happen in one single commit. It's [good practice](https://llvm.org/docs/CodeReview.html#code-reviews-speed-and-reciprocity) to keep commits small, focused on doing one single thing, especially if there is risk of revert. The risk of revert is a lot higher if N tools are modified than if only one tool is modified. It wouldn't be good to revert changes to clang-tidy just because this change breaks another unrelated tool.

Like I said, I'm not against improving the other tools, simply that it should be done in follow-up patches, one step at a time. What would be the drawback of that?

> helping them out documenting the rationale and suggesting workarounds

Sure, makes sense. Do you have any concrete suggestion in mind? Personally I can't think of a way to enable individual checks to regain access to the "full analysis". Like I said above, that would be great, but I don't know how to achieve that in practice.

If we want to protect downstream users, I suppose we can add a configuration option to revert back to the "full analysis" behavior. Would that lower the risk?

> Could you summarize what you tried and why it did not work?

I diffed which declarations were picked up with "full analysis" and with this patch, on [this example code](https://godbolt.org/z/bjs3nW6js). It appears that `IndirectFieldDecl` (which is picked up by the check) is not a **top-level declaration,** and therefore it no longer becomes part of the traversal scope. That is, this issue is unrelated to system headers or not, but rather that there are some declarations that are not top-level declarations. Why that's the case, or if it's intentional, is way beyond my knowledge.

I searched for functionality to handle these special declarations in the `ASTConsumer` class but couldn't find a suitable `handle*` function for that.

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