[PATCH] D74463: [clang-tidy] Add new check avoid-adjacent-unrelated-parameters-of-the-same-type

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal added a comment.

In D74463#1878378 <https://reviews.llvm.org/D74463#1878378>, @njames93 wrote:

> In D74463#1878290 <https://reviews.llvm.org/D74463#1878290>, @vingeldal wrote:
>
> > I am pretty sure that an option to allow short names would cause  a relatively big hit on performance (relative to how it runs without the option) for this check while also potentially causing some false negatives (which I would very much like to avoid).
>
>
> Highly unlikely, the additional name length check would only be ran on functions where the consecutive param types occur, then the check itself is very fast, you don't even need to compute the length as its stored in the identifier info, finally on cases where the name check prevents a diagnostic that is a huge performance win. The false negatives could kind of be an issue, but we could leave that up to the developer who is running the check.


I still feel a bit reluctant since the potential false positives would at least be left to the user to decide how to handle, while the false negatives from this option would just be lost without indication.
I don't see much value left in this check if one decides to use the option, anyone who would use this option might be better of just doing this check manually instead, then again it's optional of course.
If there wouldn't be much of a loss in performance I guess there isn't any significant harm in adding the option though, I'll give it a shot.

> In D74463#1878290 <https://reviews.llvm.org/D74463#1878290>, @vingeldal wrote:
> 
>> Also consider that even in the cases where the order of the parameters doesn't matter, like an averaging function, there is also the possibility to re-design to avoid this warning, by passing the parameters as some kind of collection of parameters.
> 
> 
> Forcing a developer to jump through hoops is not a great idea imo

I didn't suggest forcing anything though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74463