whisperity marked 8 inline comments as done. whisperity added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:51 + {true, 40, 50}, // Substring. + {true, 50, 66}, // Levenshtein. + {true, 75, 85}, // Jaro-Winkler. ---------------- 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... ================ Comment at: clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:271-272 + +// Checks whether ArgType is an array type identical to ParamType`s array type. +// Enforces array elements` qualifier compatibility as well. +static bool isCompatibleWithArrayReference(const QualType &ArgType, ---------------- aaron.ballman wrote: > Good catch! I believe this is what happens when you write LaTeX and code at the same time? I didn't notice this when I was tidying up the code... ================ 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) { ---------------- 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. 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