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

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this check!

We already have a frontend diagnostic for comparisons between string literals and pointers, so I'm not certain of the utility of adding a clang-tidy check for that case (see -Wstring-compare, aka, http://coliru.stacked-crooked.com/a/6f6ca7fd2f6db09a).

Comparisons against nullptr seems like it could also be handled as a frontend check as well, perhaps.


================
Comment at: clang-tidy/misc/ComparisonMisuseCheck.cpp:23
@@ +22,3 @@
+  Finder->addMatcher(
+      binaryOperator(hasEitherOperand(ignoringImpCasts(stringLiteral())),
+                     hasEitherOperand(hasType(pointsTo(isAnyCharacter()))))
----------------
Should constrain this matcher to comparison operators, no? 

================
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
----------------
hokein wrote:
> Eugene.Zelenko wrote:
> > Spaces after commas,
> Seems like your check doesn't warn any usage about the strcmp family functions.
Should remove the period at the end of this comment. Also, the w-variants for these APIs?

I don't see any code related to strcmp and friends, so perhaps this comment is spurious and should be removed?

================
Comment at: docs/clang-tidy/checks/misc-comparison-misuse.rst:1
@@ +1,2 @@
+.. title:: clang-tidy - misc-comparison-misuse
+
----------------
The code examples in the documentation should be formatted with our usual style guidelines, such as `char *` rather than `char*`, etc.

================
Comment at: docs/clang-tidy/checks/misc-comparison-misuse.rst:10
@@ +9,3 @@
+Case 1:
+  ``char*`` is compared to a string literal.
+
----------------
hokein wrote:
> Does the case cover `char[]` ?
The check is covering any character type, including `char32_t`, `wchar_t`, etc.

================
Comment at: test/clang-tidy/misc-comparison-misuse.cpp:1
@@ +1,2 @@
+// RUN: %check_clang_tidy %s misc-comparison-misuse %t
+
----------------
hokein wrote:
> Also clang-format this example.
Should be tests for other character types.


Repository:
  rL LLVM

https://reviews.llvm.org/D23427