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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.

> Also I'm convinced this check would already be of use in many code bases, as it is today, (but sure, as you said, supporting data for that would be good) and therefor it would be a clear improvement to clang-tidy.

I'm not as convinced, tbh.

> It would be better to use this check until we can do even better, rather than to hold of the check completely until we can implement it perfectly for every code base.

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.


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

https://reviews.llvm.org/D69560