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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
