Page MenuHomePhabricator

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

Authored by vingeldal on Feb 11 2020, 11:12 PM.

Details

Event Timeline

vingeldal created this revision.Feb 11 2020, 11:12 PM

Updating D74463: Add new check avoid-adjacent-unrelated-parameters-of-the-same-type

  • Changed commit message to start with [clang-tidy]

I have a feeling this check is going to throw so many false positives that it'll be too noisy to run on any real codebase.
There should be a way to silence this when it is undesired like in the example for int max(int a, int b);.
A possible solution could be based on parameter name length, usually interchangeable parameters have short names.
Maybe an option could be added IgnoreShortParamNamesSize which takes an int and disregards results if both params are shorter, set as 0 to disable the option

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidAdjacentParametersOfTheSameTypeCheck.cpp
18

You'd need to create a matcher for that

AST_MATCHER(FunctionDecl, hasMoreThan2Args) {
  return Node.getNumParams() > 2;
}
29

Please follow the naming convention of CamelCase for all Variables, same goes for all other occurances below

30

Don't use auto when the type isn't spelled out in the initialisation, the below cases are ok as they are iterators

clang-tools-extra/docs/ReleaseNotes.rst
72

A new line is needed above here

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.rst
29

New line

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.cpp
58

New line

njames93 retitled this revision from Add new check avoid-adjacent-unrelated-parameters-of-the-same-type to [clang-tidy] Add new check avoid-adjacent-unrelated-parameters-of-the-same-type.Feb 16 2020, 5:26 AM
njames93 added inline comments.Feb 16 2020, 5:31 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidAdjacentParametersOfTheSameTypeCheck.cpp
18

Sorry you'd need

AST_MATCHER(FunctionDecl, has2orMoreArgs) {
  return Node.getNumParams() >= 2;
}
vingeldal marked 2 inline comments as done.Feb 16 2020, 9:21 AM

I have a feeling this check is going to throw so many false positives that it'll be too noisy to run on any real codebase.
There should be a way to silence this when it is undesired like in the example for int max(int a, int b);.
A possible solution could be based on parameter name length, usually interchangeable parameters have short names.
Maybe an option could be added IgnoreShortParamNamesSize which takes an int and disregards results if both params are shorter, set as 0 to disable the option

I can't dispute that there will be false positives with this check, but I'm not sure there will be a problematic amount in many code bases.

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).
Since actually running the check is still optional I think it might be better to just not run this check on a code base where it gives a lot of false positives or suppress it in select header files where the false positives are plenty.
I'm feeling a bit reluctant to adding the suggested option, are you sure such an option would be worth the cost?
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.

njames93 added a comment.EditedFeb 16 2020, 11:40 AM

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.

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

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.

I am also concerned about the false positives from this check because I don't think there's going to be an easy heuristic for determining whether two identifiers are "related" to one another. There is no obvious way to silence any of the diagnostics generated as false positives short of requiring a naming convention for users to follow, which is not a particularly clean solution.

I'm not certain this is a check we should support until we solve these issues. As a use case, consider: void draw_rect(int left_corner, int top, int right_side, int bottom_part); -- how do we either not warn on this by default or how does the user tell us to not warn on it (without requiring them to jump through hoops like changing the types of the arguments)? I'd also want to see some data as to how often this check warns with true positives over a large, real-world code base (like LLVM).

vingeldal added a comment.EditedFeb 24 2020, 6:16 AM
  • how do we either not warn on this by default or how does the user tell us to not warn on it (without requiring them to jump through hoops like changing the types of the arguments)?

-I'v used comments in the source code to tell the tool to ignore cases that I'v identified as false positives. That has worked without any issues for me and I wouldn't say it's a hassle. Is that no longer supported in clang tidy or was I using another tool and just projected that memory on clang-tidy?
I'm confident that clang-format atleast has a means of locally suppressing rules.

I'd also want to see some data as to how often this check warns with true positives over a large, real-world code base (like LLVM).

-I could do this. I agree it makes sense to look at actual data on how much this detects and how much is noise.

  • how do we either not warn on this by default or how does the user tell us to not warn on it (without requiring them to jump through hoops like changing the types of the arguments)?

-I'v used comments in the source code to tell the tool to ignore cases that I'v identified as false positives. That has worked without any issues for me and I wouldn't say it's a hassle. Is that no longer supported in clang tidy or was I using another tool and just projected that memory on clang-tidy?
I'm confident that clang-format atleast has a means of locally suppressing rules.

Found what I was thinking of: "// NOLINT(cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type)"
Do you think this is too much jumping through hoops or would that suffice as a means to get rid of false positives?

  • how do we either not warn on this by default or how does the user tell us to not warn on it (without requiring them to jump through hoops like changing the types of the arguments)?

-I'v used comments in the source code to tell the tool to ignore cases that I'v identified as false positives. That has worked without any issues for me and I wouldn't say it's a hassle. Is that no longer supported in clang tidy or was I using another tool and just projected that memory on clang-tidy?
I'm confident that clang-format atleast has a means of locally suppressing rules.

Found what I was thinking of: "// NOLINT(cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type)"
Do you think this is too much jumping through hoops or would that suffice as a means to get rid of false positives?

We do support NOLINT comments in clang-tidy, but those tend to only be appropriate for code the user has control over and less appropriate for things the user often doesn't have control over. Given that this functionality is diagnosing function declarations, my concern is that a lot of the diagnostics will be in code the user cannot touch to add the markings (like system or third-party headers). Seeing how chatty the diagnostic is in practice will help determine what approach to take (maybe we need the ability to specify header files to disable the check for, maybe we need to ignore all system headers, etc).

vingeldal abandoned this revision.Feb 24 2020, 6:42 AM

Will proceed with instead contributing however I can to the more mature patch https://reviews.llvm.org/D69560