[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#1890767 <https://reviews.llvm.org/D69560#1890767>, @vingeldal wrote:

> In D69560#1890026 <https://reviews.llvm.org/D69560#1890026>, @aaron.ballman wrote:
>
> > 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.


We typically aim for checks to require as little configuration as possible by tailoring the default check behavior to be as reasonable as we can make it.

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

That is true.

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

Using the header filters would be a reasonable approach, if a bit heavy-handed. Ideally, the check would warn about what the rule wants it to warn about. I think the unsatisfying bit is the lack of clarity in the rule itself. Trying to write a checker for a rule with that much subjectivity typically leads to a lot of configuration needs.

In D69560#1890855 <https://reviews.llvm.org/D69560#1890855>, @whisperity wrote:

> In D69560#1889925 <https://reviews.llvm.org/D69560#1889925>, @vingeldal wrote:
>
> > I think this value very well may strike a good balance in that compromise but I still think it is a clear deviation from the guideline as specified.
> >  IMO all deviations from the guideline should be explicitly requested by the user, not set by default, no matter how useful that deviation is.
>
>
> While I am still planning to compile a measurement with potentially tagging false and true positives -- albeit classifying this on random projects I have no domain knowledge about is hard at first glance...


LLVM itself might be a good starting point since we've all got some passing familiarity with the project.

> @aaron.ballman Could it be a useful compromise to //not// say we are trying to solve the `cppcoreguidelines-` rule, but rather introduce this checker in another "namespace" and maybe mention in the docs that it could, although with caveats, apply "parts" of the Core Guidelines rule?

I think that could be a useful compromise, or name it `cppcoreguidelines-experimental-avoid-adjacent-parameters-of-same-type` to make it clear that we're trying things out to see what works in practice and the behavior may not match the guideline. I think the most important thing we do will be to eventually coordinate with the C++ Core Guidelines folks though. The eventual goal is for the clang-tidy check to cover what the guideline wants it to cover as its default behavior, but in a way that's going to give acceptable behavior for our users. This may mean we have a slightly chattier diagnostic than I'd like, and it may also mean that some updates to the core guideline are needed to help clarify what constitutes a "related" parameter in a way that's acceptable for analysis tools. What I want to avoid is having users say "this check doesn't match the guideline" or "we want to enable all the guidelines but we always have to disable this one because of the false positives".

Btw, we should update the terminology in the patch to use `parameter` instead of `argument` (parameters are what the function declares, arguments are what are passed in at the call site and they bind to parameters -- the issue this check is trying to solve is on the declaration side, not the caller side).


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

https://reviews.llvm.org/D69560