[PATCH] D23427: [Clang-tidy] Comparison Misuse

[Haojian Wu via cfe-commits]

hokein added inline comments.

================
Comment at: clang-tidy/misc/ComparisonMisuseCheck.cpp:31
@@ +30,3 @@
+          unless(anyOf(hasOperatorName("=="), hasOperatorName("!="))),
+          hasEitherOperand(ignoringImpCasts(gnuNullExpr())))
+          .bind("compareToNull"),
----------------
We should use `nullPointerConstant()` here to cover more null cases.

================
Comment at: clang-tidy/misc/ComparisonMisuseCheck.cpp:39
@@ +38,3 @@
+      Result.Nodes.getNodeAs<BinaryOperator>("charToLiteral");
+  if (CharToLiteral)
+    diag(CharToLiteral->getOperatorLoc(),
----------------
The code can be simplified as follows:

```
if (const auto * a = Result.Nodes.getNodeAs<BinaryOperator>("charToLiteral")) {
  ...
} else if (const auto *CompareToNull =
      Result.Nodes.getNodeAs<BinaryOperator>("compareToNull")) {
  ...
}
```

================
Comment at: clang-tidy/misc/ComparisonMisuseCheck.cpp:41
@@ +40,3 @@
+    diag(CharToLiteral->getOperatorLoc(),
+         "char* is compared to a string literal");
+
----------------
I'm wondering if we can automatically fix this case. Use `strcmp()` function is sufficient to fix it, IMO?

================
Comment at: clang-tidy/misc/ComparisonMisuseCheck.cpp:52
@@ +51,2 @@
+} // namespace clang
+
----------------
an extra blank line.

================
Comment at: clang-tidy/misc/ComparisonMisuseCheck.h:20
@@ +19,3 @@
+/// This checker reports errors related to the misuse of the comparison operator.
+/// It should warn for the following cases:
+///   - strcmp,strncmp,memcmp misuse.
----------------
s/should warn/warns

================
Comment at: clang-tidy/misc/ComparisonMisuseCheck.h:21
@@ +20,3 @@
+/// It should warn for the following cases:
+///   - strcmp,strncmp,memcmp misuse.
+///   - char* is compared to a string literal
----------------
Eugene.Zelenko wrote:
> Spaces after commas,
Seems like your check doesn't warn any usage about the strcmp family functions.

================
Comment at: docs/clang-tidy/checks/misc-comparison-misuse.rst:3
@@ +2,3 @@
+
+misc-comparison-misuse
+======================
----------------
Please also update `docs/ReleaseNotes.rst`.

================
Comment at: docs/clang-tidy/checks/misc-comparison-misuse.rst:10
@@ +9,3 @@
+Case 1:
+  ``char*`` is compared to a string literal.
+
----------------
Does the case cover `char[]` ?

================
Comment at: test/clang-tidy/misc-comparison-misuse.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s misc-comparison-misuse %t
+
----------------
Also clang-format this example.

================
Comment at: test/clang-tidy/misc-comparison-misuse.cpp:13
@@ +12,3 @@
+void test_null_to_pointer(int *p){
+  if (NULL>=p);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: comparison to nullptr [misc-comparison-misuse]
----------------
Add `if (p <= NULL);` test case.


Repository:
  rL LLVM

https://reviews.llvm.org/D23427