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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits