Page MenuHomePhabricator

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

Authored by whisperity on Oct 29 2019, 6:13 AM.

Details

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

Diff Detail

Event Timeline

whisperity created this revision.Oct 29 2019, 6:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 29 2019, 6:13 AM
whisperity added a comment.EditedOct 29 2019, 6:16 AM

A few interesting "true positive" findings:

  • (Such cases with many bools are being discussed on enhancing type strength)
  • (Personally, I found this case a rather big find.)

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

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:

Eugene.Zelenko added inline comments.Oct 29 2019, 7:23 AM
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 edited subscribers, added: baloghadamsoftware, NoQ; removed: Szelethus.

@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 marked 11 inline comments as done.Oct 31 2019, 3:00 AM
whisperity added inline comments.
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.

  • 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 edited the summary of this revision. (Show Details)
  • Removed This check from the documentation comment of the check's class too.
NoQ added inline comments.Nov 4 2019, 5:00 PM
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).