whisperity added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:31-32 + + char DissimilarBelow; + char SimilarAbove; + ---------------- aaron.ballman wrote: > `signed char` since we're doing `> -1` below? Or better yet, `int8_t` because > these aren't really characters? Oh right, I always forget `char` isn't guaranteed. `int8_t` seems like a better idea anyways (until we have `int6_t`...) ================ Comment at: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:51 + {true, 40, 50}, // Substring. + {true, 50, 66}, // Levenshtein. + {true, 75, 85}, // Jaro-Winkler. ---------------- varjujan wrote: > whisperity wrote: > > aaron.ballman wrote: > > > We should probably document where all these numbers come from, but `66` > > > definitely jumps out at me as being a bit strange. :-D > > Unfortunately, I have absolutely no idea. All these values are percentages > > between 0 and 100 (`-1` is just saying that //"This heuristic doesn't > > accept percentages"//), and this is written in the documentation now. > > However, the answer to //"**why** 66%?"//, unless @varjujan can say > > something, I think is lost to history... > > > > I'll read over his thesis once again, maybe I can find anything with > > regards to this. > > > > Either way, I've detailed from both the code and the thesis how the > > percentages are meant. In some cases, the % is calculated as //"% of the > > longer string's length"//. In the Leventhstein's case, it's actually > > inverted: > > > > ``` > > Dist = (1 - Dist / LongerLength) * 100; > > ``` > > > > So what this says is that if the current arg1-param1 arg2-param2 pairing > > has less than the inverse of 50% (which is more than 50%) of the longer > > string's edit distance, but the arg2-param1 and arg1-param2 (the suggested > > swapped order) has more than the inverse of 66% (which is less than 33%), > > then the swap will be suggested. > > > > Originally these values were called `LowerBound` and `UpperBound`, > > respectively, which was saying **even less** about what they mean... > Sadly, I think there isn't any scientific reason behind these numbers. They > just looked ok after a couple of test runs. (Maybe they make sense for > shorter arg names, like the 66 for 3 char long names.) In [Rice2017], the inflexion point of the precision/recall plot is at around `0.55`-ish threshold. They express this threshold as the distance of distances, i.e. `0` would mean the (a1, p1) - (a2, p2) pair is good as it is, and `1` would mean that it should **definitely** be (a2, p1) - (a1, p2) instead. ================ Comment at: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:347 +// Checks whether ArgType converts implicitly to ParamType. +static bool areTypesCompatible(QualType ArgType, QualType ParamType, + const ASTContext &Ctx) { ---------------- whisperity wrote: > aaron.ballman wrote: > > It seems like we're doing an awful lot of the same work as > > `ASTContext::typesAreCompatible()` and type compatibility rules are pretty > > complex, so I worry about this implementation being different than the > > `ASTContext` implementation. Have you explored whether we can reuse more of > > the logic from `ASTContext` here, or are they doing fundamentally different > > kinds of type compatibility checks? > No, I didn't know that function even existed. This check must be older than > that function. Actually, no, that function is pretty old... However, that function, and all the function it subsequently calls, require a **non-const** `ASTContext`. I have changed `ASTContext`: ``` ╰─ git diff --cached --stat clang/include/clang/AST/ASTContext.h | 67 ++++++++++++++++++++++++++++++++++++------------------------------- clang/lib/AST/ASTContext.cpp | 72 +++++++++++++++++++++++++++++++++++++----------------------------------- 2 files changed, 73 insertions(+), 66 deletions(-) ``` making related member functions and internal static functions take `const ASTContext &`/`*`. This, by itself, did not break any of the tests of `check-clang check-clang-unit check-clang-tools check-clang-extra-unit`! ================ Comment at: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:347-349 +static bool areTypesCompatible(QualType ArgType, QualType ParamType, + const ASTContext &Ctx) { + if (ArgType.isNull() || ParamType.isNull()) ---------------- whisperity wrote: > whisperity wrote: > > aaron.ballman wrote: > > > It seems like we're doing an awful lot of the same work as > > > `ASTContext::typesAreCompatible()` and type compatibility rules are > > > pretty complex, so I worry about this implementation being different than > > > the `ASTContext` implementation. Have you explored whether we can reuse > > > more of the logic from `ASTContext` here, or are they doing fundamentally > > > different kinds of type compatibility checks? > > No, I didn't know that function even existed. This check must be older than > > that function. > Actually, no, that function is pretty old... However, that function, and all > the function it subsequently calls, require a **non-const** `ASTContext`. I > have changed `ASTContext`: > > ``` > ╰─ git diff --cached --stat > clang/include/clang/AST/ASTContext.h | > 67 ++++++++++++++++++++++++++++++++++++------------------------------- > clang/lib/AST/ASTContext.cpp | > 72 +++++++++++++++++++++++++++++++++++++----------------------------------- > 2 files changed, 73 insertions(+), 66 deletions(-) > ``` > > making related member functions and internal static functions take `const > ASTContext &`/`*`. > > This, by itself, did not break any of the tests of `check-clang > check-clang-unit check-clang-tools check-clang-extra-unit`! @aaron.ballman Changing the function to be ``` return Ctx.typesAreCompatible(ArgType, ParamType); ``` will make the checker miss the test case about `T`/`const T&` mixup. ``` void value_const_reference(int llllll, const int& kkkkkk); void const_ref_value_swapped() { const int& kkkkkk = 42; const int& llllll = 42; value_const_reference(kkkkkk, llllll); // error: CHECK-MESSAGES: expected string not found in input: // warning: 1st argument 'kkkkkk' (passed to 'llllll') looks like it might be swapped with the 2nd, 'llllll' (passed to 'kkkkkk') } ``` ---- Setting `bool CompareUnqualified = true` (3rd argument to `typesAreCompatible`) doesn't help either. Which is valid from `typesAreCompatible`'s perspective... that function answer the question, applied to the context of the above test: //"Is `llllll = kkkkkk;` valid?"//, which is obviously **false** as both are `const T&`s. ================ Comment at: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.h:38 + enum class Bound { + /// The percentage below which the heuristic deems the strings + /// dissimilar. ---------------- aaron.ballman wrote: > The comment is somewhat confusing because the enumerators have the values 0 > and 1, which are valid percentages but not likely what the comment means. I'll rename it to `BoundKind` and update the comments. 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