[PATCH] D69560: [clang-tidy] Add 'cppcoreguidelines-avoid-adjacent-arguments-of-same-type' check

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal added a comment.

In D69560#1890026 <https://reviews.llvm.org/D69560#1890026>, @aaron.ballman wrote:

> In D69560#1889925 <https://reviews.llvm.org/D69560#1889925>, @vingeldal wrote:
>
> > Doesn't clang-tidy exclude warnings from system headers by default though?
>
>
> Third-party SDKs are not always brought in as system headers, unfortunately. Even ignoring system and third-party headers, having two parameters of the same type is a natural occurrence for some data types (like `bool`) where it is going to be extremely hard to tell whether they're related or unrelated parameters.
>
> > And there is always the possibility of using multiple .clang-tidy files in the code base to use this check for only selected portions of the code base.
>
> That is an option, but it would be better for the check to be usable without requiring a lot of custom configuration.


One can't really expect a ones own .clang-tidy file to be applicable to all third party code used in ones project. It is a fact, with or without this check, that if you use third party libraries you can't expect the third party libraries to comply with your own .clang-tidy file. You should expect to set up a separate .clang-tidy file for all third party libraries used by your project with or without this check -and that's ok because it is arguably not a big effort to do so.

> Incremental improvement is always a good thing, but it has to start from a stable base. I'm not yet convinced this check meets the bar for inclusion in clang-tidy. There is no proposed way to tell how two parameters relate to one another, and the simple enforcement seems dangerously simple.
> 
>> Would it not be a reasonable approach to accept this implementation and improve `NOLINT` to work on entire files or lists of files rather than squeezing more heuristics (prone to false negatives) to the check?
> 
> AFAICT, that would be a novel new use of `NOLINT`, so I'm wary of it. I think the first thing is to gather some data as to how this check performs as-is. If the behavior is unreasonable, we can see what ways in which it is unreasonable with the dataset and see if we can tease out either heuristics or configuration options that would make it more palatable. We may also want to rope some of the C++ Core Guidelines authors in on the discussion at that point because that would be data suggesting their suggested simple enforcement is untenable.

There is also the --header-filter command line option to configure the tool to skip some headers.

To sum up my position:
I'm all for investigating the performance of this check and basing any decisions on data. Until we have that data I just want to challenge the claim that there are no suitable ways to turn the check of for select portions of the code base,
there are definitely ways to do that which both work satisfyingly and are in no way novel or unconventional.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69560/new/

https://reviews.llvm.org/D69560