[clang-tidy] Create a check for signed and unsigned integers comparison

clang-tools-extra/clang-tidy/modernize/CMakeLists.txt

@@ -36,6 +36,7 @@ add_clang_library(clangTidyModernizeModule STATIC
   UseEmplaceCheck.cpp
   UseEqualsDefaultCheck.cpp
   UseEqualsDeleteCheck.cpp
+  UseIntegerSignComparisonCheck.cpp
   UseNodiscardCheck.cpp
   UseNoexceptCheck.cpp
   UseNullptrCheck.cpp

clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp

@@ -37,6 +37,7 @@
 #include "UseEmplaceCheck.h"
 #include "UseEqualsDefaultCheck.h"
 #include "UseEqualsDeleteCheck.h"
+#include "UseIntegerSignComparisonCheck.h"
 #include "UseNodiscardCheck.h"
 #include "UseNoexceptCheck.h"
 #include "UseNullptrCheck.h"
@@ -76,6 +77,8 @@ class ModernizeModule : public ClangTidyModule {
     CheckFactories.registerCheck<PassByValueCheck>("modernize-pass-by-value");
     CheckFactories.registerCheck<UseDesignatedInitializersCheck>(
         "modernize-use-designated-initializers");
+    CheckFactories.registerCheck<UseIntegerSignComparisonCheck>(
+        "modernize-use-integer-sign-comparison");
     CheckFactories.registerCheck<UseRangesCheck>("modernize-use-ranges");
     CheckFactories.registerCheck<UseStartsEndsWithCheck>(
         "modernize-use-starts-ends-with");

clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp

@@ -0,0 +1,171 @@
+//===--- UseIntegerSignComparisonCheck.cpp - clang-tidy -------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "UseIntegerSignComparisonCheck.h"
+#include "clang/AST/Expr.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+using namespace clang::ast_matchers::internal;
+
+namespace clang::tidy::modernize {
+
+/// Find if the passed type is the actual "char" type,
+/// not applicable to explicit "signed char" or "unsigned char" types.
+static bool isActualCharType(const clang::QualType &Ty) {
+  using namespace clang;
+  const Type *DesugaredType = Ty->getUnqualifiedDesugaredType();
+  if (const auto *BT = llvm::dyn_cast<BuiltinType>(DesugaredType))
+    return (BT->getKind() == BuiltinType::Char_U ||
+            BT->getKind() == BuiltinType::Char_S);
+  return false;
+}
+
+namespace {
+AST_MATCHER(clang::QualType, isActualChar) {
+  return clang::tidy::modernize::isActualCharType(Node);
+}
+} // namespace
+
+static BindableMatcher<clang::Stmt>
+intCastExpression(bool IsSigned,
+                  const std::string &CastBindName = std::string()) {
+  // std::cmp_{} functions trigger a compile-time error if either LHS or RHS
+  // is a non-integer type, char, enum or bool
+  // (unsigned char/ signed char are Ok and can be used).
+  auto IntTypeExpr = expr(hasType(hasCanonicalType(qualType(
+      isInteger(), IsSigned ? isSignedInteger() : isUnsignedInteger(),
+      unless(isActualChar()), unless(booleanType()), unless(enumType())))));
+
+  const auto ImplicitCastExpr =
+      CastBindName.empty() ? implicitCastExpr(hasSourceExpression(IntTypeExpr))
+                           : implicitCastExpr(hasSourceExpression(IntTypeExpr))
+                                 .bind(CastBindName);
+
+  const auto CStyleCastExpr = cStyleCastExpr(has(ImplicitCastExpr));
+  const auto StaticCastExpr = cxxStaticCastExpr(has(ImplicitCastExpr));
+  const auto FunctionalCastExpr = cxxFunctionalCastExpr(has(ImplicitCastExpr));
+
+  return expr(anyOf(ImplicitCastExpr, CStyleCastExpr, StaticCastExpr,
+                    FunctionalCastExpr));
+}
+
+static StringRef parseOpCode(BinaryOperator::Opcode Code) {
+  switch (Code) {
+  case BO_LT:
+    return "cmp_less";
+  case BO_GT:
+    return "cmp_greater";
+  case BO_LE:
+    return "cmp_less_equal";
+  case BO_GE:
+    return "cmp_greater_equal";
+  case BO_EQ:
+    return "cmp_equal";
+  case BO_NE:
+    return "cmp_not_equal";
+  default:
+    return "";
+  }
+}
+
+UseIntegerSignComparisonCheck::UseIntegerSignComparisonCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
+                                               utils::IncludeSorter::IS_LLVM),
+                      areDiagsSelfContained()) {}
+
+void UseIntegerSignComparisonCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IncludeStyle", IncludeInserter.getStyle());
+}
+
+void UseIntegerSignComparisonCheck::registerMatchers(MatchFinder *Finder) {
+  const auto SignedIntCastExpr = intCastExpression(true, "sIntCastExpression");
+  const auto UnSignedIntCastExpr = intCastExpression(false);
+
+  // Flag all operators "==", "<=", ">=", "<", ">", "!="
+  // that are used between signed/unsigned
+  const auto CompareOperator =
+      binaryOperator(hasAnyOperatorName("==", "<=", ">=", "<", ">", "!="),
+                     hasOperands(SignedIntCastExpr, UnSignedIntCastExpr),
+                     unless(isInTemplateInstantiation()))
+          .bind("intComparison");
+
+  Finder->addMatcher(CompareOperator, this);
+}
+
+void UseIntegerSignComparisonCheck::registerPPCallbacks(
+    const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
+  IncludeInserter.registerPreprocessor(PP);
+}
+
+void UseIntegerSignComparisonCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *SignedCastExpression =
+      Result.Nodes.getNodeAs<ImplicitCastExpr>("sIntCastExpression");
+  assert(SignedCastExpression);
+
+  // Ignore the match if we know that the signed int value is not negative.
+  Expr::EvalResult EVResult;
+  if (!SignedCastExpression->isValueDependent() &&
+      SignedCastExpression->getSubExpr()->EvaluateAsInt(EVResult,
+                                                        *Result.Context)) {
+    const llvm::APSInt SValue = EVResult.Val.getInt();
+    if (SValue.isNonNegative())
+      return;
+  }
+
+  const auto *BinaryOp =
+      Result.Nodes.getNodeAs<BinaryOperator>("intComparison");
+  if (BinaryOp == nullptr)
+    return;
+
+  const BinaryOperator::Opcode OpCode = BinaryOp->getOpcode();
+
+  const Expr *LHS = BinaryOp->getLHS()->IgnoreImpCasts();
+  const Expr *RHS = BinaryOp->getRHS()->IgnoreImpCasts();
+  if (LHS == nullptr || RHS == nullptr)
+    return;
+  const Expr *SubExprLHS = nullptr;
+  const Expr *SubExprRHS = nullptr;
+  SourceRange R1 = SourceRange(LHS->getBeginLoc());
+  SourceRange R2 = SourceRange(BinaryOp->getOperatorLoc());
+  SourceRange R3 = SourceRange(Lexer::getLocForEndOfToken(
+      RHS->getEndLoc(), 0, *Result.SourceManager, getLangOpts()));
+  if (const auto *LHSCast = llvm::dyn_cast<ExplicitCastExpr>(LHS)) {
+    SubExprLHS = LHSCast->getSubExpr();
+    R1 = SourceRange(LHS->getBeginLoc(),
+                     SubExprLHS->getBeginLoc().getLocWithOffset(-1));
+    R2.setBegin(Lexer::getLocForEndOfToken(
+        SubExprLHS->getEndLoc(), 0, *Result.SourceManager, getLangOpts()));
+  }
+  if (const auto *RHSCast = llvm::dyn_cast<ExplicitCastExpr>(RHS)) {
+    SubExprRHS = RHSCast->getSubExpr();
+    R2.setEnd(SubExprRHS->getBeginLoc().getLocWithOffset(-1));
+  }
+  DiagnosticBuilder Diag =
+      diag(BinaryOp->getBeginLoc(),
+           "comparison between 'signed' and 'unsigned' integers");
+  const std::string CmpNamespace = ("std::" + parseOpCode(OpCode)).str();
+  const std::string CmpHeader = "<utility>";
+  // Prefer modernize-use-integer-sign-comparison when C++20 is available!
+  Diag << FixItHint::CreateReplacement(
+      CharSourceRange(R1, SubExprLHS != nullptr),
+      llvm::Twine(CmpNamespace + "(").str());
+  Diag << FixItHint::CreateReplacement(R2, ",");
+  Diag << FixItHint::CreateReplacement(CharSourceRange::getCharRange(R3), ")");
+
+  // If there is no include for cmp_{*} functions, we'll add it.
+  Diag << IncludeInserter.createIncludeInsertion(
+      Result.SourceManager->getFileID(BinaryOp->getBeginLoc()), CmpHeader);
+}
+
+} // namespace clang::tidy::modernize

clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.h

@@ -0,0 +1,42 @@
+//===--- UseIntegerSignComparisonCheck.h - clang-tidy -----------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEINTEGERSIGNCOMPARISONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEINTEGERSIGNCOMPARISONCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "../utils/IncludeInserter.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+namespace clang::tidy::modernize {
+
+/// Replace comparisons between signed and unsigned integers with their safe
+/// C++20 ``std::cmp_*`` alternative, if available.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize/use-integer-sign-comparison.html
+class UseIntegerSignComparisonCheck : public ClangTidyCheck {
+public:
+  UseIntegerSignComparisonCheck(StringRef Name, ClangTidyContext *Context);
+
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus20;
+  }
+
+private:
+  utils::IncludeInserter IncludeInserter;
+};
+
+} // namespace clang::tidy::modernize
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEINTEGERSIGNCOMPARISONCHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -123,10 +123,16 @@ New checks
   Gives warnings for tagged unions, where the number of tags is
   different from the number of data members inside the union.
 
+- New :doc:`modernize-use-integer-sign-comparison
+  <clang-tidy/checks/modernize/use-integer-sign-comparison>` check.
+
+  Replace comparisons between signed and unsigned integers with their safe
+  C++20 ``std::cmp_*`` alternative, if available.
+
 - New :doc:`portability-template-virtual-member-function
   <clang-tidy/checks/portability/template-virtual-member-function>` check.
 
-  Finds cases when an uninstantiated virtual member function in a template class 
+  Finds cases when an uninstantiated virtual member function in a template class
   causes cross-compiler incompatibility.
 
 New check aliases

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -300,6 +300,7 @@ Clang-Tidy Checks
    :doc:`modernize-use-emplace <modernize/use-emplace>`, "Yes"
    :doc:`modernize-use-equals-default <modernize/use-equals-default>`, "Yes"
    :doc:`modernize-use-equals-delete <modernize/use-equals-delete>`, "Yes"
+   :doc:`modernize-use-integer-sign-comparison <modernize/use-integer-sign-comparison>`, "Yes"
    :doc:`modernize-use-nodiscard <modernize/use-nodiscard>`, "Yes"
    :doc:`modernize-use-noexcept <modernize/use-noexcept>`, "Yes"
    :doc:`modernize-use-nullptr <modernize/use-nullptr>`, "Yes"

clang-tools-extra/docs/clang-tidy/checks/modernize/use-integer-sign-comparison.rst

@@ -0,0 +1,36 @@
+.. title:: clang-tidy - modernize-use-integer-sign-comparison
+
+modernize-use-integer-sign-comparison
+=====================================
+
+Replace comparisons between signed and unsigned integers with their safe
+C++20 ``std::cmp_*`` alternative, if available.
+
+The check provides a replacement only for C++20 or later, otherwise
+it highlights the problem and expects the user to fix it manually.
+
+Examples of fixes created by the check:
+
+.. code-block:: c++
+
+  unsigned int func(int a, unsigned int b) {
+    return a == b;
+  }
+
+becomes
+
+.. code-block:: c++
+
+  #include <utility>
+
+  unsigned int func(int a, unsigned int b) {
+    return std::cmp_equal(a, b);
+  }
+
+Options
+-------
+
+.. option:: IncludeStyle
+
+  A string specifying which include-style is used, `llvm` or `google`.
+  Default is `llvm`.