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

[Gábor Horváth via cfe-commits]

Xazax-hun wrote:

> 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 see. I think we should the very least investigate this. Open a ticket if you plan to land this change to make sure it is not forgotten, have a FIXME and so on. Manage it properly because this approach introduces technical debt. My problem is, statements like "way beyond my knowledge" makes me feel that you have no plans to address these in the future which makes me less OK to be on board with a change that introduces debt that needs to be cleaned up later. 

> 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.

E.g., in case of the `IndirectFieldDecl` we could ask users to match on the top level tag declaration instead if that works. @steakhal was against the checks implicitly triggering the "full analysis". But I think we should figure out why those decls are not considered top level and if it was not intentional, fix it.

> I prefer to start from this patch.

It would make sense if it was really incremental, but it is not. Currently, there is a consensus that it would be better to have this functionality in the ASTMatchers. Landing code only to throw it out is not incremental. Incremental is when the next patch builds on the previous one. What is described here is more iterative. I still fail to understand what is the push back from moving the code to the proper component. I heard no technical arguments, only vague claims that it would be more code or there would be more pushback from the owners.

Could we at least try? If it is really more code and if there is really a push back we can always revert back to this approach. But what prevents us from at least trying? 

> The current patch is still unstable in my opinion.

If that is the case that is an even stronger argument to not land it but let it cook a bit more. 

> Just like the incubator, when it has been tested by a certain number of users and is stable enough, then we can consider moving it to the ast-matcher.

I don't understand this. Tidy is the most popular user of matchers. If this is not good enough for the matchers, it should not be good enough for tidy either. And again, what is the technical argument here? Are we just lazy trying to move this code to the ast matchers or is there any particular reason why tidy is better suited for "incubation"? Also, in the matcher library we would have an option. So turning it on only for tidy would give the same results. Am I missing something here? 

I think at the point we spent more time arguing here than it would have been to just move the code to the ASTMatcher library.

Arguments like "we already have this code" are not technical in nature. 

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