aaron.ballman added a comment.
Pinging @alexfh for opinions about the check (especially any concerns about
true or false positive rates). I continue to think this is a good check that's
worth moving forward on.
================
Comment at:
clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:31-32
+
+ char DissimilarBelow;
+ char SimilarAbove;
+
----------------
`signed char` since we're doing `> -1` below? Or better yet, `int8_t` because
these aren't really characters?
================
Comment at:
clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:51
+ {true, 40, 50}, // Substring.
+ {true, 50, 66}, // Levenshtein.
+ {true, 75, 85}, // Jaro-Winkler.
----------------
We should probably document where all these numbers come from, but `66`
definitely jumps out at me as being a bit strange. :-D
================
Comment at:
clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:93
+/// Returns how many % X is of Y.
+static inline double percentage(double X, double Y) { return X / Y * 100; }
+
----------------
================
Comment at:
clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:102-108
+ if (AbbreviationDictionary.find(Arg) != AbbreviationDictionary.end())
+ if (Param.equals(AbbreviationDictionary.lookup(Arg)))
+ return true;
+
+ if (AbbreviationDictionary.find(Param) != AbbreviationDictionary.end())
+ if (Arg.equals(AbbreviationDictionary.lookup(Param)))
+ return true;
----------------
================
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,
----------------
================
Comment at:
clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:296
+
+// Checks if multilevel pointers` qualifiers compatibility continues on the
+// current pointer level.
----------------
================
Comment at:
clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:301-302
+// and if PramType has a cv-qualifier that's not in ArgType, then every * in
+// ParamType to the right
+// of that cv-qualifier, except the last one, must also be const-qualified.
+static bool arePointersStillQualCompatible(QualType ArgType, QualType
ParamType,
----------------
Can re-flow the comments.
================
Comment at:
clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:341
+
+ } while (ParamType->isAnyPointerType() && ArgType->isAnyPointerType());
+ // The final type does not match, or pointer levels differ.
----------------
Elsewhere we're using `isPointerType()` which is subtly different because it
excludes ObjC object pointers. We should be consistent about the usage.
================
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) {
----------------
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?
================
Comment at:
clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:462
+ : ClangTidyCheck(Name, Context) {
+ const auto &GetToggleOpt = [this](Heuristic H) -> bool {
+ auto Idx = static_cast<std::size_t>(H);
----------------
================
Comment at:
clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.cpp:467
+ };
+ const auto &GetBoundOpt = [this](Heuristic H, Bound B) -> char {
+ auto Idx = static_cast<std::size_t>(H);
----------------
================
Comment at:
clang-tools-extra/clang-tidy/readability/SuspiciousCallArgumentCheck.h:38
+ enum class Bound {
+ /// The percentage below which the heuristic deems the strings
+ /// dissimilar.
----------------
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.
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
