[PATCH] D20689: [clang-tidy] Add 'readability-suspicious-call-argument' check

[Whisperity via Phabricator via cfe-commits]

whisperity marked an inline comment as done.
whisperity added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:657-660
+    } else {
+      ParamTypes.push_back(QualType());
+      ParamNames.push_back(StringRef());
+    }
----------------
aaron.ballman wrote:
> Can this case happen?
Oops... It seems I posted the updated patch right where you were writing more comments and we got into a data race. Which case are you referring to? It's now affixed to a `diag(` call for me...


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-suspicious-call-argument.rst:61-68
+The *prefix* heuristic reports if one of the strings is a sufficiently long
+prefix of the other string, e.g. ``target`` to ``targetPtr``.
+The similarity percentage is the length ratio of the prefix to the longer
+string, in the previous example, it would be `6 / 9 = 66.66...`\%.
+
+This heuristic can be configured with :ref:`bounds<opt_Bounds>`.
+The default bounds are: below `25`\% dissimilar and above `30`\% similar.
----------------
aaron.ballman wrote:
> I wonder how Hungarian notation impacts this heuristic -- I would imagine a lot of similar prefixes in such a code base, and things like `lpstr` as a prefix could be a pretty large chunk of some identifiers.
The switch is only warned if it would be type-safe. If the HN prefix is in both //the same way//, then it could be ignored. Thus, given `f(const char* lpszFoo, const char* lpszBar, uint16_t psnzXXX)  {}`, if I do a `f(lpszX, lpszA, ...);`, it should consider in both cases that the prefix is common and matches. Note that to produce a diagnostic, **two** things has to be proven: first, that the //current// ordering is dissimilar (below threshold A), and second, that the potential swapped ordering is more similar (above threshold B).


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/readability-suspicious-call-argument.rst:75
+In the case of ``oldValue`` and ``value`` compared, the similarity percentage
+is `8 / 5 = 62.5`\%.
+
----------------
aaron.ballman wrote:
> Similar to above, I wonder how numeric digits impact this heuristic -- do the defaults consider this to be a swap?
> ```
> void foo(int frobble1, int frobble2);
> foo(frobble2, frobble1); // Hopefully identified as a swap
> foo(bar2, bar1); // How about this?
> ```
Currently, neither of these are matched. I have to look into why the first isn't... it really should, based on the "equality" heuristic. It's too trivial.

The second... well... that's trickier. I would say it shouldn't match, because if it did, we would be swamped with false positives. The suffix is only 1 character, and we need 25/30% based on the string's length.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D20689