[Clang-tidy] Comparison Misuse

[xazax.hun]

Summary

This checker warns for the misuse of comparison operators

• char* is compared to a string literal
• inequality operator usage for NULL

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

Please run Clang-format on newly added files. Test case is definitely needs it.

clang-tidy/misc/ComparisonMisuseCheck.h

19

Check, please.

21

Spaces after commas,

docs/clang-tidy/checks/misc-comparison-misuse.rst

6

Check, please.

13

Clang-format examples, please.

[Eugene.Zelenko]

clang-tidy/misc/ComparisonMisuseCheck.cpp

21

Please remove this line.

[hokein]

clang-tidy/misc/ComparisonMisuseCheck.cpp

31

We should use nullPointerConstant() here to cover more null cases.

39

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")) {
  ...
}

41

I'm wondering if we can automatically fix this case. Use strcmp() function is sufficient to fix it, IMO?

52

an extra blank line.

clang-tidy/misc/ComparisonMisuseCheck.h

20

s/should warn/warns

21

Seems like your check doesn't warn any usage about the strcmp family functions.

docs/clang-tidy/checks/misc-comparison-misuse.rst

3

Please also update docs/ReleaseNotes.rst.

10

Does the case cover char[] ?

test/clang-tidy/misc-comparison-misuse.cpp

1

Also clang-format this example.

13

Add if (p <= NULL); test case.

[aaron.ballman]

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.

clang-tidy/misc/ComparisonMisuseCheck.cpp

23

Should constrain this matcher to comparison operators, no?

clang-tidy/misc/ComparisonMisuseCheck.h

21

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?

docs/clang-tidy/checks/misc-comparison-misuse.rst

1

The code examples in the documentation should be formatted with our usual style guidelines, such as char * rather than char*, etc.

10

The check is covering any character type, including char32_t, wchar_t, etc.

test/clang-tidy/misc-comparison-misuse.cpp

1

Should be tests for other character types.

[xazax.hun]

For the first case ToT clang compiler gives a warning (-Wstring-compare), for the second case, it generates a compiler error (error: ordered comparison between pointer and zero). Note that, older versions of clang did not even give a warning for that case.

It looks like this check no longer makes sense considering the current warnings and errors of clang top of tree.

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   MisplacedConstCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
+  ComparisonMisuseCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   DanglingHandleCheck.cpp
   DefinitionsInHeadersCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundings.cpp
   InefficientAlgorithmCheck.cpp

clang-tidy/misc/ComparisonMisuseCheck.h

+//===--- ComparisonMisuseCheck.h - clang-tidy--------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COMPARISON_MISUSE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COMPARISON_MISUSE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// This checker reports errors related to the misuse of the comparison operator.
+/// It should warn for the following cases:
+///   - strcmp,strncmp,memcmp misuse.
+///   - char* is compared to a string literal
+///   - inequality operator usage for NULL
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-comparison-misuse.html
+class ComparisonMisuseCheck : public ClangTidyCheck {
+public:
+  ComparisonMisuseCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COMPARISON_MISUSE_H

clang-tidy/misc/ComparisonMisuseCheck.cpp

+//===--- ComparisonMisuseCheck.cpp - clang-tidy----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ComparisonMisuseCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void ComparisonMisuseCheck::registerMatchers(MatchFinder *Finder) {
+
+  Finder->addMatcher(
+      binaryOperator(hasEitherOperand(ignoringImpCasts(stringLiteral())),
+                     hasEitherOperand(hasType(pointsTo(isAnyCharacter()))))
+          .bind("charToLiteral"),
+      this);
+
+  Finder->addMatcher(
+      binaryOperator(
+          unless(anyOf(hasOperatorName("=="), hasOperatorName("!="))),
+          hasEitherOperand(ignoringImpCasts(gnuNullExpr())))
+          .bind("compareToNull"),
+      this);
+}
+
+void ComparisonMisuseCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *CharToLiteral =
+      Result.Nodes.getNodeAs<BinaryOperator>("charToLiteral");
+  if (CharToLiteral)
+    diag(CharToLiteral->getOperatorLoc(),
+         "char* is compared to a string literal");
+
+  const auto *CompareToNull =
+      Result.Nodes.getNodeAs<BinaryOperator>("compareToNull");
+  if (CompareToNull)
+    diag(CompareToNull->getOperatorLoc(), "comparison to nullptr");
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+

clang-tidy/misc/MiscTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
 #include "MisplacedConstCheck.h"
 #include "UnconventionalAssignOperatorCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
+#include "ComparisonMisuseCheck.h"
 #include "DanglingHandleCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AssertSideEffectCheck>(
         "misc-assert-side-effect");
     CheckFactories.registerCheck<MisplacedConstCheck>(
         "misc-misplaced-const");
     CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
         "misc-unconventional-assign-operator");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "misc-bool-pointer-implicit-conversion");
+    CheckFactories.registerCheck<ComparisonMisuseCheck>(
+        "misc-comparison-misuse");
     CheckFactories.registerCheck<DanglingHandleCheck>(
         "misc-dangling-handle");
     CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
         "misc-definitions-in-headers");
     CheckFactories.registerCheck<FoldInitTypeCheck>(
         "misc-fold-init-type");
     CheckFactories.registerCheck<ForwardDeclarationNamespaceCheck>(
         "misc-forward-declaration-namespace");

docs/clang-tidy/checks/list.rst

.. toctree::
    google-runtime-references
    llvm-header-guard
    llvm-include-order
    llvm-namespace-comment
    llvm-twine-local
    misc-argument-comment
    misc-assert-side-effect
    misc-bool-pointer-implicit-conversion
+   misc-comparison-misuse
    misc-dangling-handle
    misc-definitions-in-headers
    misc-fold-init-type
    misc-forward-declaration-namespace
    misc-inaccurate-erase
    misc-incorrect-roundings
    misc-inefficient-algorithm
    misc-macro-parentheses

docs/clang-tidy/checks/misc-comparison-misuse.rst

+.. title:: clang-tidy - misc-comparison-misuse
+
+misc-comparison-misuse
+======================
+
+This checker reports errors related to the misuse of the comparison operator.
+It should warn for the following cases:
+
+Case 1:
+  ``char*`` is compared to a string literal.
+
+.. code-block::
+   bool isMyString(const char * my){
+    return "mystring"==my;//error. comparing pointer to string literal
+   }
+
+
+Case 2:
+  Inequality operator usage for ``NULL``.
+
+.. code-block:: c++
+   void(int * p){
+     if (NULL>=p)//error, use only NULL==p, NULL!=p
+   }
+

test/clang-tidy/misc-comparison-misuse.cpp

+// RUN: %check_clang_tidy %s misc-comparison-misuse %t
+
+#define NULL __null
+
+bool test_pointer_to_literal(const char *my){
+  bool b = (my=="mystring");
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: char* is compared to a string literal [misc-comparison-misuse]
+  return "mystring"==my;
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: char* is compared to a string literal [misc-comparison-misuse]
+}
+
+void test_null_to_pointer(int *p){
+  if (NULL>=p);
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: comparison to nullptr [misc-comparison-misuse]
+  
+  if (NULL==p);
+  
+  if (NULL!=p);
+}
+