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

[whisperity]

Summary

Finds function definitions where arguments of the same type follow each other directly, making call sites prone to calling the function with swapped or badly ordered arguments.

The relevant I.24 C++ Core Guidelines rule to which conformity is checked can be found at http://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#i24-avoid-adjacent-unrelated-parameters-of-the-same-type

[whisperity]

A few interesting "true positive" findings:

• AnalysisDeclContext.cpp_9aaed563ddd9ebd73fdd228c2883b8e7.plist.html226 KBDownload
  (Such cases with many bools are being discussed on enhancing type strength)
• bitcoingui.cpp_27769deaa63c894e4fc430d7122c368d.plist.html257 KBDownload
  (Personally, I found this case a rather big find.)
• CheckerRegistry.cpp_d0d988840a154a07670cc410e23a7c8e.plist.html226 KBDownload
• text_format.cc_f34502df2725b30018c086b6767cbd26.plist.html373 KBDownload

Of course, an "interface conformity check" will find "false positive" cases which cannot be proved or done away by a heuristic easily:

FP_substitute.cc_7bdfe1e94aaced3286a6be8172a92884.plist.html209 KBDownload

I attach a render of the check's documentation so it's easy to read if you don't want to manually build the docs:

clang-tidy - cppcoreguidelines-avoid-adjacent-arguments-of-same-type — Extra Clang Tools 10 documentation (2019-10-29 13_59_17).html18 KBDownload

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp

2

Please adjust length to 80 characters.

10

Please C++ headers aster LLVM ones.

36

Functions should be static. Anonymous namespace only for types definitions.

489

Check seems to be useful for C and probably for Objective-C.

clang-tools-extra/docs/ReleaseNotes.rst

94

Please remove This check.

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

7

Please remove This check.

57

Please use single back-tick for option values.

66

Please use single back-tick for option values.

126

Please add new line.

[whisperity]

@Szelethus and @baloghadamsoftware are colleagues to me whom are far more knowledgeable about check development and I want them to see that I want a review from them. I specifically didn't do an "internal with colleagues" downstream review with regards to this code.

clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp

489

I'm not knowledgeable about Objective-C at all to make a decision on how the "adjacent argument ranges" could be calculated and what mixups are possible. As for C, should a cppcoreguidelines- check be enabled for C? Or you mean we should allow it to work, and the user will toggle how they see fit.

[whisperity]

clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp

489

I've added a FIXME for ObjC as I'm really not qualified in that language. C support has been added.

[whisperity]

• Fix some comments, formatting and documentation
• Organised test files to be in the same directory as others on the Monorepo structure
• Helper functions moved from namespace (anonymous) to static.
• Added test for C and enabled check for C code.

[whisperity]

• Removed This check from the documentation comment of the check's class too.

[NoQ]

clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp

489

The check should keep working on Objective-C to check C-style calls, and it should also keep working on Objective-C++ to check C++-style method calls. You should be able to test this by renaming any of your .c/.cpp test files to .m/.mm (of course you don't *have* to duplicate your tests; just keep a couple of functions to see that the warning is still there).

However, the check shouldn't try to check Objective-C message expressions / method declarations, because Objective-C message syntax has so-called "parameter labels": the caller is forced to spell out which parameter it's passing, which effectively mitigates the problem.

Note that ObjCMethodDecl is not a sub-class of FunctionDecl, so your checker is already working correctly (as long as you remove the explicit suppression).