[PATCH] D20689: [clang-tidy] Add 'readability-suspicious-call-argument' check

[Aaron Ballman via Phabricator via cfe-commits]

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