Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

qt-tatiana
Copy link
Contributor

@qt-tatiana qt-tatiana commented Oct 21, 2024

  • modernize-use-integer-sign-comparison replaces comparisons between signed and unsigned integers with their safe C++20 std::cmp_* alternative, if available.

Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot
Copy link
Member

llvmbot commented Oct 21, 2024

@llvm/pr-subscribers-clang-tidy

Author: None (qt-tatiana)

Changes
  • modernize-use-integer-sign-comparison check performs comparisons between signed and unsigned integer types mathematically correct. If C++20 is supported the check replaces integers comparisons to std::cmp_{} functions and add <utility> include.

  • add a qt module for qt-related checks.

  • qt-integer-sign-comparison check works like an alias of modernize-use-integer-sign-comparison for C++20.
    If only C++17 is supported the check replaces integers comparisons to q20::cmp_{} functions and add <QtCore/q20utility.h> include. The check assumes the analysed code is Qt-based code.


Patch is 24.72 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/113144.diff

14 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/CMakeLists.txt (+2)
  • (modified) clang-tools-extra/clang-tidy/ClangTidyForceLinker.h (+5)
  • (modified) clang-tools-extra/clang-tidy/modernize/CMakeLists.txt (+1)
  • (added) clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp (+169)
  • (added) clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.h (+46)
  • (added) clang-tools-extra/clang-tidy/qt/CMakeLists.txt (+26)
  • (added) clang-tools-extra/clang-tidy/qt/QtTidyModule.cpp (+46)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+17)
  • (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+3)
  • (added) clang-tools-extra/docs/clang-tidy/checks/modernize/use-integer-sign-comparison.rst (+43)
  • (added) clang-tools-extra/docs/clang-tidy/checks/qt/IntegerCrossSignComparison.rst (+64)
  • (modified) clang-tools-extra/docs/clang-tidy/index.rst (+1)
  • (added) clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp (+61)
  • (added) clang-tools-extra/test/clang-tidy/checkers/qt/qt-integer-sign-comparison.cpp (+61)
diff --git a/clang-tools-extra/clang-tidy/CMakeLists.txt b/clang-tools-extra/clang-tidy/CMakeLists.txt
index 83a3236131dc93..56ef16a8fb37d6 100644
--- a/clang-tools-extra/clang-tidy/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/CMakeLists.txt
@@ -75,6 +75,7 @@ add_subdirectory(objc)
 add_subdirectory(openmp)
 add_subdirectory(performance)
 add_subdirectory(portability)
+add_subdirectory(qt)
 add_subdirectory(readability)
 add_subdirectory(zircon)
 set(ALL_CLANG_TIDY_CHECKS
@@ -99,6 +100,7 @@ set(ALL_CLANG_TIDY_CHECKS
   clangTidyOpenMPModule
   clangTidyPerformanceModule
   clangTidyPortabilityModule
+  clangTidyQtModule
   clangTidyReadabilityModule
   clangTidyZirconModule
   )
diff --git a/clang-tools-extra/clang-tidy/ClangTidyForceLinker.h b/clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
index adde9136ff1dd5..3c777f42520223 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
@@ -127,6 +127,11 @@ extern volatile int PortabilityModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED PortabilityModuleAnchorDestination =
     PortabilityModuleAnchorSource;
 
+// This anchor is used to force the linker to link the QtClangTidyModule.
+extern volatile int QtClangTidyModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED QtClangTidyModuleAnchorDestination =
+    QtClangTidyModuleAnchorSource;
+
 // This anchor is used to force the linker to link the ReadabilityModule.
 extern volatile int ReadabilityModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED ReadabilityModuleAnchorDestination =
diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
index c919d49b42873a..bab1167fb15ff2 100644
--- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
+++ b/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
diff --git a/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp
new file mode 100644
index 00000000000000..8f394a14a9b0c4
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp
@@ -0,0 +1,169 @@
+//===--- 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 {
+
+UseIntegerSignComparisonCheck::UseIntegerSignComparisonCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
+                                               utils::IncludeSorter::IS_LLVM),
+                      areDiagsSelfContained()),
+      IsQtApplication(Options.get("IsQtApplication", false)) {}
+
+void UseIntegerSignComparisonCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IsQtApplication", IsQtApplication);
+}
+
+void UseIntegerSignComparisonCheck::registerMatchers(MatchFinder *Finder) {
+  const auto SignedIntCastExpr = intCastExpression(true, "sIntCastExpression");
+  const auto UnSignedIntCastExpr =
+      intCastExpression(false, "uIntCastExpression");
+
+  // Flag all operators "==", "<=", ">=", "<", ">", "!="
+  // that are used between signed/unsigned
+  const auto CompareOperator =
+      expr(binaryOperator(hasAnyOperatorName("==", "<=", ">=", "<", ">", "!="),
+                          anyOf(allOf(hasLHS(SignedIntCastExpr),
+                                      hasRHS(UnSignedIntCastExpr)),
+                                allOf(hasLHS(UnSignedIntCastExpr),
+                                      hasRHS(SignedIntCastExpr)))))
+          .bind("intComparison");
+
+  Finder->addMatcher(CompareOperator, this);
+}
+
+BindableMatcher<clang::Stmt> UseIntegerSignComparisonCheck::intCastExpression(
+    bool IsSigned, const std::string &CastBindName) const {
+  auto IntTypeExpr = expr();
+  if (IsSigned) {
+    IntTypeExpr = expr(hasType(qualType(isInteger(), isSignedInteger())));
+  } else {
+    IntTypeExpr =
+        expr(hasType(qualType(isInteger(), unless(isSignedInteger()))));
+  }
+
+  const auto ImplicitCastExpr =
+      implicitCastExpr(hasSourceExpression(IntTypeExpr)).bind(CastBindName);
+
+  const auto CStyleCastExpr = cStyleCastExpr(has(ImplicitCastExpr));
+  const auto StaticCastExpr = cxxStaticCastExpr(has(ImplicitCastExpr));
+  const auto FunctionalCastExpr = cxxFunctionalCastExpr(has(ImplicitCastExpr));
+
+  return traverse(TK_AsIs, expr(anyOf(ImplicitCastExpr, CStyleCastExpr,
+                                      StaticCastExpr, FunctionalCastExpr)));
+}
+
+std::string
+UseIntegerSignComparisonCheck::parseOpCode(BinaryOperator::Opcode code) const {
+  switch (code) {
+  case BO_LT:
+    return std::string("cmp_less");
+  case BO_GT:
+    return std::string("cmp_greater");
+  case BO_LE:
+    return std::string("cmp_less_equal");
+  case BO_GE:
+    return std::string("cmp_greater_equal");
+  case BO_EQ:
+    return std::string("cmp_equal");
+  case BO_NE:
+    return std::string("cmp_not_equal");
+  default:
+    return std::string();
+  }
+}
+
+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");
+  const auto *UnSignedCastExpression =
+      Result.Nodes.getNodeAs<ImplicitCastExpr>("uIntCastExpression");
+  assert(SignedCastExpression);
+  assert(UnSignedCastExpression);
+
+  // 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)) {
+    llvm::APSInt SValue = EVResult.Val.getInt();
+    if (SValue.isNonNegative())
+      return;
+  }
+
+  const auto *BinaryOp =
+      Result.Nodes.getNodeAs<BinaryOperator>("intComparison");
+  if (BinaryOp == nullptr)
+    return;
+
+  auto OpCode = BinaryOp->getOpcode();
+  const auto *LHS = BinaryOp->getLHS()->IgnoreParenImpCasts();
+  const auto *RHS = BinaryOp->getRHS()->IgnoreParenImpCasts();
+  if (LHS == nullptr || RHS == nullptr)
+    return;
+
+  StringRef LHSString(Lexer::getSourceText(
+      CharSourceRange::getTokenRange(LHS->getSourceRange()),
+      *Result.SourceManager, getLangOpts()));
+
+  StringRef RHSString(Lexer::getSourceText(
+      CharSourceRange::getTokenRange(RHS->getSourceRange()),
+      *Result.SourceManager, getLangOpts()));
+
+  DiagnosticBuilder Diag =
+      diag(BinaryOp->getBeginLoc(),
+           "comparison between 'signed' and 'unsigned' integers");
+
+  if (!(getLangOpts().CPlusPlus17 && IsQtApplication) &&
+      !getLangOpts().CPlusPlus20)
+    return;
+
+  std::string CmpNamespace;
+  std::string CmpInclude;
+  if (getLangOpts().CPlusPlus17 && IsQtApplication) {
+    CmpInclude = "<QtCore/q20utility.h>";
+    CmpNamespace = std::string("q20::") + parseOpCode(OpCode);
+  }
+
+  if (getLangOpts().CPlusPlus20) {
+    CmpInclude = "<utility>";
+    CmpNamespace = std::string("std::") + parseOpCode(OpCode);
+  }
+
+  // Use qt-use-integer-sign-comparison when C++17 is available and only for Qt
+  // apps. Prefer modernize-use-integer-sign-comparison when C++20 is available!
+  Diag << FixItHint::CreateReplacement(
+      CharSourceRange::getTokenRange(BinaryOp->getBeginLoc(),
+                                     BinaryOp->getEndLoc()),
+      StringRef(std::string(CmpNamespace) + std::string("(") +
+                std::string(LHSString) + std::string(", ") +
+                std::string(RHSString) + std::string(")")));
+
+  // If there is no include for cmp_{*} functions, we'll add it.
+  Diag << IncludeInserter.createIncludeInsertion(
+      Result.SourceManager->getFileID(BinaryOp->getBeginLoc()),
+      StringRef(CmpInclude));
+}
+
+} // namespace clang::tidy::modernize
diff --git a/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.h b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.h
new file mode 100644
index 00000000000000..13c6ea9f74ec32
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.h
@@ -0,0 +1,46 @@
+//===--- 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 {
+
+/// Class detects comparisons between signed and unsigned integers
+///
+/// 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.CPlusPlus;
+  }
+
+private:
+  ast_matchers::internal::BindableMatcher<clang::Stmt>
+  intCastExpression(bool IsSigned, const std::string &CastBindName) const;
+  std::string parseOpCode(BinaryOperator::Opcode code) const;
+
+  utils::IncludeInserter IncludeInserter;
+  bool IsQtApplication = false;
+};
+
+} // namespace clang::tidy::modernize
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEINTEGERSIGNCOMPARISONCHECK_H
diff --git a/clang-tools-extra/clang-tidy/qt/CMakeLists.txt b/clang-tools-extra/clang-tidy/qt/CMakeLists.txt
new file mode 100644
index 00000000000000..5133c47b3031ee
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/qt/CMakeLists.txt
@@ -0,0 +1,26 @@
+set(LLVM_LINK_COMPONENTS
+    FrontendOpenMP
+    Support
+)
+
+add_clang_library(clangTidyQtModule
+  QtTidyModule.cpp
+
+  LINK_LIBS
+  clangTidy
+  clangTidyModernizeModule
+  clangTidyUtils
+
+  DEPENDS
+  omp_gen
+  ClangDriverOptions
+  )
+
+clang_target_link_libraries(clangTidyQtModule
+  PRIVATE
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTooling
+  )
diff --git a/clang-tools-extra/clang-tidy/qt/QtTidyModule.cpp b/clang-tools-extra/clang-tidy/qt/QtTidyModule.cpp
new file mode 100644
index 00000000000000..3f2c5ec9a4fa21
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/qt/QtTidyModule.cpp
@@ -0,0 +1,46 @@
+//===-- QtTidyModule.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 "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+#include "../modernize/UseIntegerSignComparisonCheck.h"
+
+namespace clang::tidy {
+namespace qt {
+
+/// A module containing checks of the C++ Core Guidelines
+class QtClangTidyModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<modernize::UseIntegerSignComparisonCheck>(
+        "qt-integer-sign-comparison");
+  }
+
+  ClangTidyOptions getModuleOptions() override {
+    ClangTidyOptions Options;
+    ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
+
+    Opts["qt-integer-sign-comparison."
+         "IsQtApplication"] = "true";
+
+    return Options;
+  }
+};
+
+// Register the LLVMTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<QtClangTidyModule>
+    X("qt-module", "Adds checks for the Qt framework Guidelines.");
+
+} // namespace qt
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the QtClangTidyModule.
+volatile int QtClangTidyModuleAnchorSource = 0;
+
+} // namespace clang::tidy
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index e8148e06b6af28..ba9922b835d230 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -129,6 +129,15 @@ New checks
   Finds cases when an uninstantiated virtual member function in a template class 
   causes cross-compiler incompatibility.
 
+- New :doc:`modernize-use-integer-sign-comparison
+  <clang-tidy/checks/modernize/use-integer-sign-comparison>` check.
+
+  Performs comparisons between signed and unsigned integer types
+  mathematically correct. If C++20 is supported a fix-it replaces
+  integers comparisons to
+  std::cmp_{equal,not_equal,{less,greater}{,_equal}} functions.
+
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
@@ -136,6 +145,14 @@ New check aliases
   :doc:`bugprone-sizeof-expression
   <clang-tidy/checks/bugprone/sizeof-expression>` was added.
 
+- New alias :doc:`qt-integer-sign-comparison
+  <clang-tidy/checks/qt/integer-sign-comparison>` to
+  doc:`modernize-use-integer-sign-comparison
+  <clang-tidy/checks/modernize/use-integer-sign-comparison>` check.
+  If C++17 is supported, the fix-it replaces integers comparisons to
+  q20::cmp_{equal,not_equal,{less,greater}{,_equal}} functions.
+  The check assumes the analysed code is Qt-based code.
+
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 0082234f5ed31b..f1b9f032c76615 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -30,6 +30,7 @@ Clang-Tidy Checks
    openmp/*
    performance/*
    portability/*
+   qt/*
    readability/*
    zircon/*
 
@@ -300,6 +301,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"
@@ -348,6 +350,7 @@ Clang-Tidy Checks
    :doc:`portability-restrict-system-includes <portability/restrict-system-includes>`, "Yes"
    :doc:`portability-simd-intrinsics <portability/simd-intrinsics>`,
    :doc:`portability-std-allocator-const <portability/std-allocator-const>`,
+   :doc:`qt-integer-sign-comparison <qt/integer-sign-comparison>`, "Yes"
    :doc:`portability-template-virtual-member-function <portability/template-virtual-member-function>`,
    :doc:`readability-avoid-const-params-in-decls <readability/avoid-const-params-in-decls>`, "Yes"
    :doc:`readability-avoid-nested-conditional-operator <readability/avoid-nested-conditional-operator>`,
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-integer-sign-comparison.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-integer-sign-comparison.rst
new file mode 100644
index 00000000000000..21bee8ec16b17d
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-integer-sign-comparison.rst
@@ -0,0 +1,43 @@
+.. title:: clang-tidy - modernize-use-integer-sign-comparison
+
+modernize-use-integer-sign-comparison
+=====================================
+
+The check detects comparison between signed and unsigned integer values.
+If C++20 is supported, the check suggests a fix-it.
+
+Examples of fixes created by the check:
+
+.. code-block:: c++
+
+    uint func(int bla)
+    {
+        uint result;
+        if (result == bla)
+            return 0;
+
+        return 1;
+    }
+
+becomes
+
+.. code-block:: c++
+
+    #include <utility>
+
+    uint func(int bla)
+    {
+        uint result;
+        if (std::cmp_equal(result, bla))
+            return 0;
+
+        return 1;
+    }
+
+Options
+-------
+
+.. option:: IncludeStyle
+
+   A string specifying which include-style is used, `llvm` or `google`. Default
+   is `llvm`.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/qt/IntegerCrossSignComparison.rst b/clang-tools-extra/docs/clang-tidy/checks/qt/IntegerCrossSignComparison.rst
new file mode 100644
index 00000000000000..1f9e1089826ce9
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/qt/IntegerCrossSignComparison.rst
@@ -0,0 +1,64 @@
+.. title:: clang-tidy - qt-integer-sign-comparison
+
+qt-integer-sign-comparison
+=============================
+
+The check detects comparison between signed and unsigned integer values.
+If C++20 is supported, the check suggests a std related fix-it.
+If C++17 is supported, the check suggests a Qt related fix-it.
+
+Examples of fixes created by the check:
+
+.. code-block:: c++
+
+    uint func(int bla)
+    {
+        uint result;
+        if (result == bla)
+            return 0;
+
+        return 1;
+    }
+
+in C++17 becomes
+
+.. code-block:: c++
+
+    <QtCore/q20utility.h>
+
+    uint func(int bla)
+    {
+        uint result;
+        if (q20::cmp_equal(result, bla))
+            return 0;
+
+        return 1;
+    }
+
+in C++20 becomes
+
+.. code-block:: c++
+
+    #include <utility>
+
+    uint func(int bla)
+    {
+        uint result;
+        if (std::cmp_equal(result, bla))
+            return 0;
+
+        return 1;
+    }
+
+Options
+-------
+
+.. option:: IncludeStyle
+
+   A string specifying which include-style is used, `llvm` or `google`. Default
+   is `llvm`.
+
+.. option:: IsQtApplication
+
+  When `true` (default `false`), then it is assumed that the code being analyzed
+  is the Qt-based code.
diff --git a/clang-tools-extra/docs/clang-tidy/index.rst b/clang-tools-extra/docs/clang-tidy/index.rst
index a4233d5d8e2694..9663ed03c8734d 100644
--- a/clang-tools-extra/docs/clang-tidy/index.rst
+++ b/clang-tools-extra/docs/clang-tidy/index.rst
@@ -84,6 +84,7 @@ Name prefix            Description
 ``performance-``       Checks that target performance-related issues.
 ``portability-``       Checks that target portability-related issues that don't
                        relate to any particular coding style.
+``qt-``                Checks related to Qt framework.
 ``readability-``       Checks that target readability-related issues that don't
                        relate to any particular coding style.
 ``zircon-``            Checks related to Zircon kernel coding conventions.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp
new file mode 100644
index 00000000000000..b580e2dfc6731d
--- /dev/null
+++ b/c...
[truncated]

@llvmbot
Copy link
Member

llvmbot commented Oct 21, 2024

@llvm/pr-subscribers-clang-tools-extra

Author: None (qt-tatiana)

Changes
  • modernize-use-integer-sign-comparison check performs comparisons between signed and unsigned integer types mathematically correct. If C++20 is supported the check replaces integers comparisons to std::cmp_{} functions and add <utility> include.

  • add a qt module for qt-related checks.

  • qt-integer-sign-comparison check works like an alias of modernize-use-integer-sign-comparison for C++20.
    If only C++17 is supported the check replaces integers comparisons to q20::cmp_{} functions and add <QtCore/q20utility.h> include. The check assumes the analysed code is Qt-based code.


Patch is 24.72 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/113144.diff

14 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/CMakeLists.txt (+2)
  • (modified) clang-tools-extra/clang-tidy/ClangTidyForceLinker.h (+5)
  • (modified) clang-tools-extra/clang-tidy/modernize/CMakeLists.txt (+1)
  • (added) clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp (+169)
  • (added) clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.h (+46)
  • (added) clang-tools-extra/clang-tidy/qt/CMakeLists.txt (+26)
  • (added) clang-tools-extra/clang-tidy/qt/QtTidyModule.cpp (+46)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+17)
  • (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+3)
  • (added) clang-tools-extra/docs/clang-tidy/checks/modernize/use-integer-sign-comparison.rst (+43)
  • (added) clang-tools-extra/docs/clang-tidy/checks/qt/IntegerCrossSignComparison.rst (+64)
  • (modified) clang-tools-extra/docs/clang-tidy/index.rst (+1)
  • (added) clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp (+61)
  • (added) clang-tools-extra/test/clang-tidy/checkers/qt/qt-integer-sign-comparison.cpp (+61)
diff --git a/clang-tools-extra/clang-tidy/CMakeLists.txt b/clang-tools-extra/clang-tidy/CMakeLists.txt
index 83a3236131dc93..56ef16a8fb37d6 100644
--- a/clang-tools-extra/clang-tidy/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/CMakeLists.txt
@@ -75,6 +75,7 @@ add_subdirectory(objc)
 add_subdirectory(openmp)
 add_subdirectory(performance)
 add_subdirectory(portability)
+add_subdirectory(qt)
 add_subdirectory(readability)
 add_subdirectory(zircon)
 set(ALL_CLANG_TIDY_CHECKS
@@ -99,6 +100,7 @@ set(ALL_CLANG_TIDY_CHECKS
   clangTidyOpenMPModule
   clangTidyPerformanceModule
   clangTidyPortabilityModule
+  clangTidyQtModule
   clangTidyReadabilityModule
   clangTidyZirconModule
   )
diff --git a/clang-tools-extra/clang-tidy/ClangTidyForceLinker.h b/clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
index adde9136ff1dd5..3c777f42520223 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
@@ -127,6 +127,11 @@ extern volatile int PortabilityModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED PortabilityModuleAnchorDestination =
     PortabilityModuleAnchorSource;
 
+// This anchor is used to force the linker to link the QtClangTidyModule.
+extern volatile int QtClangTidyModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED QtClangTidyModuleAnchorDestination =
+    QtClangTidyModuleAnchorSource;
+
 // This anchor is used to force the linker to link the ReadabilityModule.
 extern volatile int ReadabilityModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED ReadabilityModuleAnchorDestination =
diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
index c919d49b42873a..bab1167fb15ff2 100644
--- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
+++ b/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
diff --git a/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp
new file mode 100644
index 00000000000000..8f394a14a9b0c4
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.cpp
@@ -0,0 +1,169 @@
+//===--- 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 {
+
+UseIntegerSignComparisonCheck::UseIntegerSignComparisonCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
+                                               utils::IncludeSorter::IS_LLVM),
+                      areDiagsSelfContained()),
+      IsQtApplication(Options.get("IsQtApplication", false)) {}
+
+void UseIntegerSignComparisonCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IsQtApplication", IsQtApplication);
+}
+
+void UseIntegerSignComparisonCheck::registerMatchers(MatchFinder *Finder) {
+  const auto SignedIntCastExpr = intCastExpression(true, "sIntCastExpression");
+  const auto UnSignedIntCastExpr =
+      intCastExpression(false, "uIntCastExpression");
+
+  // Flag all operators "==", "<=", ">=", "<", ">", "!="
+  // that are used between signed/unsigned
+  const auto CompareOperator =
+      expr(binaryOperator(hasAnyOperatorName("==", "<=", ">=", "<", ">", "!="),
+                          anyOf(allOf(hasLHS(SignedIntCastExpr),
+                                      hasRHS(UnSignedIntCastExpr)),
+                                allOf(hasLHS(UnSignedIntCastExpr),
+                                      hasRHS(SignedIntCastExpr)))))
+          .bind("intComparison");
+
+  Finder->addMatcher(CompareOperator, this);
+}
+
+BindableMatcher<clang::Stmt> UseIntegerSignComparisonCheck::intCastExpression(
+    bool IsSigned, const std::string &CastBindName) const {
+  auto IntTypeExpr = expr();
+  if (IsSigned) {
+    IntTypeExpr = expr(hasType(qualType(isInteger(), isSignedInteger())));
+  } else {
+    IntTypeExpr =
+        expr(hasType(qualType(isInteger(), unless(isSignedInteger()))));
+  }
+
+  const auto ImplicitCastExpr =
+      implicitCastExpr(hasSourceExpression(IntTypeExpr)).bind(CastBindName);
+
+  const auto CStyleCastExpr = cStyleCastExpr(has(ImplicitCastExpr));
+  const auto StaticCastExpr = cxxStaticCastExpr(has(ImplicitCastExpr));
+  const auto FunctionalCastExpr = cxxFunctionalCastExpr(has(ImplicitCastExpr));
+
+  return traverse(TK_AsIs, expr(anyOf(ImplicitCastExpr, CStyleCastExpr,
+                                      StaticCastExpr, FunctionalCastExpr)));
+}
+
+std::string
+UseIntegerSignComparisonCheck::parseOpCode(BinaryOperator::Opcode code) const {
+  switch (code) {
+  case BO_LT:
+    return std::string("cmp_less");
+  case BO_GT:
+    return std::string("cmp_greater");
+  case BO_LE:
+    return std::string("cmp_less_equal");
+  case BO_GE:
+    return std::string("cmp_greater_equal");
+  case BO_EQ:
+    return std::string("cmp_equal");
+  case BO_NE:
+    return std::string("cmp_not_equal");
+  default:
+    return std::string();
+  }
+}
+
+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");
+  const auto *UnSignedCastExpression =
+      Result.Nodes.getNodeAs<ImplicitCastExpr>("uIntCastExpression");
+  assert(SignedCastExpression);
+  assert(UnSignedCastExpression);
+
+  // 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)) {
+    llvm::APSInt SValue = EVResult.Val.getInt();
+    if (SValue.isNonNegative())
+      return;
+  }
+
+  const auto *BinaryOp =
+      Result.Nodes.getNodeAs<BinaryOperator>("intComparison");
+  if (BinaryOp == nullptr)
+    return;
+
+  auto OpCode = BinaryOp->getOpcode();
+  const auto *LHS = BinaryOp->getLHS()->IgnoreParenImpCasts();
+  const auto *RHS = BinaryOp->getRHS()->IgnoreParenImpCasts();
+  if (LHS == nullptr || RHS == nullptr)
+    return;
+
+  StringRef LHSString(Lexer::getSourceText(
+      CharSourceRange::getTokenRange(LHS->getSourceRange()),
+      *Result.SourceManager, getLangOpts()));
+
+  StringRef RHSString(Lexer::getSourceText(
+      CharSourceRange::getTokenRange(RHS->getSourceRange()),
+      *Result.SourceManager, getLangOpts()));
+
+  DiagnosticBuilder Diag =
+      diag(BinaryOp->getBeginLoc(),
+           "comparison between 'signed' and 'unsigned' integers");
+
+  if (!(getLangOpts().CPlusPlus17 && IsQtApplication) &&
+      !getLangOpts().CPlusPlus20)
+    return;
+
+  std::string CmpNamespace;
+  std::string CmpInclude;
+  if (getLangOpts().CPlusPlus17 && IsQtApplication) {
+    CmpInclude = "<QtCore/q20utility.h>";
+    CmpNamespace = std::string("q20::") + parseOpCode(OpCode);
+  }
+
+  if (getLangOpts().CPlusPlus20) {
+    CmpInclude = "<utility>";
+    CmpNamespace = std::string("std::") + parseOpCode(OpCode);
+  }
+
+  // Use qt-use-integer-sign-comparison when C++17 is available and only for Qt
+  // apps. Prefer modernize-use-integer-sign-comparison when C++20 is available!
+  Diag << FixItHint::CreateReplacement(
+      CharSourceRange::getTokenRange(BinaryOp->getBeginLoc(),
+                                     BinaryOp->getEndLoc()),
+      StringRef(std::string(CmpNamespace) + std::string("(") +
+                std::string(LHSString) + std::string(", ") +
+                std::string(RHSString) + std::string(")")));
+
+  // If there is no include for cmp_{*} functions, we'll add it.
+  Diag << IncludeInserter.createIncludeInsertion(
+      Result.SourceManager->getFileID(BinaryOp->getBeginLoc()),
+      StringRef(CmpInclude));
+}
+
+} // namespace clang::tidy::modernize
diff --git a/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.h b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.h
new file mode 100644
index 00000000000000..13c6ea9f74ec32
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/UseIntegerSignComparisonCheck.h
@@ -0,0 +1,46 @@
+//===--- 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 {
+
+/// Class detects comparisons between signed and unsigned integers
+///
+/// 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.CPlusPlus;
+  }
+
+private:
+  ast_matchers::internal::BindableMatcher<clang::Stmt>
+  intCastExpression(bool IsSigned, const std::string &CastBindName) const;
+  std::string parseOpCode(BinaryOperator::Opcode code) const;
+
+  utils::IncludeInserter IncludeInserter;
+  bool IsQtApplication = false;
+};
+
+} // namespace clang::tidy::modernize
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEINTEGERSIGNCOMPARISONCHECK_H
diff --git a/clang-tools-extra/clang-tidy/qt/CMakeLists.txt b/clang-tools-extra/clang-tidy/qt/CMakeLists.txt
new file mode 100644
index 00000000000000..5133c47b3031ee
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/qt/CMakeLists.txt
@@ -0,0 +1,26 @@
+set(LLVM_LINK_COMPONENTS
+    FrontendOpenMP
+    Support
+)
+
+add_clang_library(clangTidyQtModule
+  QtTidyModule.cpp
+
+  LINK_LIBS
+  clangTidy
+  clangTidyModernizeModule
+  clangTidyUtils
+
+  DEPENDS
+  omp_gen
+  ClangDriverOptions
+  )
+
+clang_target_link_libraries(clangTidyQtModule
+  PRIVATE
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTooling
+  )
diff --git a/clang-tools-extra/clang-tidy/qt/QtTidyModule.cpp b/clang-tools-extra/clang-tidy/qt/QtTidyModule.cpp
new file mode 100644
index 00000000000000..3f2c5ec9a4fa21
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/qt/QtTidyModule.cpp
@@ -0,0 +1,46 @@
+//===-- QtTidyModule.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 "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+#include "../modernize/UseIntegerSignComparisonCheck.h"
+
+namespace clang::tidy {
+namespace qt {
+
+/// A module containing checks of the C++ Core Guidelines
+class QtClangTidyModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<modernize::UseIntegerSignComparisonCheck>(
+        "qt-integer-sign-comparison");
+  }
+
+  ClangTidyOptions getModuleOptions() override {
+    ClangTidyOptions Options;
+    ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;
+
+    Opts["qt-integer-sign-comparison."
+         "IsQtApplication"] = "true";
+
+    return Options;
+  }
+};
+
+// Register the LLVMTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<QtClangTidyModule>
+    X("qt-module", "Adds checks for the Qt framework Guidelines.");
+
+} // namespace qt
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the QtClangTidyModule.
+volatile int QtClangTidyModuleAnchorSource = 0;
+
+} // namespace clang::tidy
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index e8148e06b6af28..ba9922b835d230 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -129,6 +129,15 @@ New checks
   Finds cases when an uninstantiated virtual member function in a template class 
   causes cross-compiler incompatibility.
 
+- New :doc:`modernize-use-integer-sign-comparison
+  <clang-tidy/checks/modernize/use-integer-sign-comparison>` check.
+
+  Performs comparisons between signed and unsigned integer types
+  mathematically correct. If C++20 is supported a fix-it replaces
+  integers comparisons to
+  std::cmp_{equal,not_equal,{less,greater}{,_equal}} functions.
+
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
@@ -136,6 +145,14 @@ New check aliases
   :doc:`bugprone-sizeof-expression
   <clang-tidy/checks/bugprone/sizeof-expression>` was added.
 
+- New alias :doc:`qt-integer-sign-comparison
+  <clang-tidy/checks/qt/integer-sign-comparison>` to
+  doc:`modernize-use-integer-sign-comparison
+  <clang-tidy/checks/modernize/use-integer-sign-comparison>` check.
+  If C++17 is supported, the fix-it replaces integers comparisons to
+  q20::cmp_{equal,not_equal,{less,greater}{,_equal}} functions.
+  The check assumes the analysed code is Qt-based code.
+
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 0082234f5ed31b..f1b9f032c76615 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -30,6 +30,7 @@ Clang-Tidy Checks
    openmp/*
    performance/*
    portability/*
+   qt/*
    readability/*
    zircon/*
 
@@ -300,6 +301,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"
@@ -348,6 +350,7 @@ Clang-Tidy Checks
    :doc:`portability-restrict-system-includes <portability/restrict-system-includes>`, "Yes"
    :doc:`portability-simd-intrinsics <portability/simd-intrinsics>`,
    :doc:`portability-std-allocator-const <portability/std-allocator-const>`,
+   :doc:`qt-integer-sign-comparison <qt/integer-sign-comparison>`, "Yes"
    :doc:`portability-template-virtual-member-function <portability/template-virtual-member-function>`,
    :doc:`readability-avoid-const-params-in-decls <readability/avoid-const-params-in-decls>`, "Yes"
    :doc:`readability-avoid-nested-conditional-operator <readability/avoid-nested-conditional-operator>`,
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-integer-sign-comparison.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-integer-sign-comparison.rst
new file mode 100644
index 00000000000000..21bee8ec16b17d
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-integer-sign-comparison.rst
@@ -0,0 +1,43 @@
+.. title:: clang-tidy - modernize-use-integer-sign-comparison
+
+modernize-use-integer-sign-comparison
+=====================================
+
+The check detects comparison between signed and unsigned integer values.
+If C++20 is supported, the check suggests a fix-it.
+
+Examples of fixes created by the check:
+
+.. code-block:: c++
+
+    uint func(int bla)
+    {
+        uint result;
+        if (result == bla)
+            return 0;
+
+        return 1;
+    }
+
+becomes
+
+.. code-block:: c++
+
+    #include <utility>
+
+    uint func(int bla)
+    {
+        uint result;
+        if (std::cmp_equal(result, bla))
+            return 0;
+
+        return 1;
+    }
+
+Options
+-------
+
+.. option:: IncludeStyle
+
+   A string specifying which include-style is used, `llvm` or `google`. Default
+   is `llvm`.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/qt/IntegerCrossSignComparison.rst b/clang-tools-extra/docs/clang-tidy/checks/qt/IntegerCrossSignComparison.rst
new file mode 100644
index 00000000000000..1f9e1089826ce9
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/qt/IntegerCrossSignComparison.rst
@@ -0,0 +1,64 @@
+.. title:: clang-tidy - qt-integer-sign-comparison
+
+qt-integer-sign-comparison
+=============================
+
+The check detects comparison between signed and unsigned integer values.
+If C++20 is supported, the check suggests a std related fix-it.
+If C++17 is supported, the check suggests a Qt related fix-it.
+
+Examples of fixes created by the check:
+
+.. code-block:: c++
+
+    uint func(int bla)
+    {
+        uint result;
+        if (result == bla)
+            return 0;
+
+        return 1;
+    }
+
+in C++17 becomes
+
+.. code-block:: c++
+
+    <QtCore/q20utility.h>
+
+    uint func(int bla)
+    {
+        uint result;
+        if (q20::cmp_equal(result, bla))
+            return 0;
+
+        return 1;
+    }
+
+in C++20 becomes
+
+.. code-block:: c++
+
+    #include <utility>
+
+    uint func(int bla)
+    {
+        uint result;
+        if (std::cmp_equal(result, bla))
+            return 0;
+
+        return 1;
+    }
+
+Options
+-------
+
+.. option:: IncludeStyle
+
+   A string specifying which include-style is used, `llvm` or `google`. Default
+   is `llvm`.
+
+.. option:: IsQtApplication
+
+  When `true` (default `false`), then it is assumed that the code being analyzed
+  is the Qt-based code.
diff --git a/clang-tools-extra/docs/clang-tidy/index.rst b/clang-tools-extra/docs/clang-tidy/index.rst
index a4233d5d8e2694..9663ed03c8734d 100644
--- a/clang-tools-extra/docs/clang-tidy/index.rst
+++ b/clang-tools-extra/docs/clang-tidy/index.rst
@@ -84,6 +84,7 @@ Name prefix            Description
 ``performance-``       Checks that target performance-related issues.
 ``portability-``       Checks that target portability-related issues that don't
                        relate to any particular coding style.
+``qt-``                Checks related to Qt framework.
 ``readability-``       Checks that target readability-related issues that don't
                        relate to any particular coding style.
 ``zircon-``            Checks related to Zircon kernel coding conventions.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-integer-sign-comparison.cpp
new file mode 100644
index 00000000000000..b580e2dfc6731d
--- /dev/null
+++ b/c...
[truncated]

@EugeneZelenko
Copy link
Contributor

@qt-tatiana: Please do not forget to resolve fixed comments.

@qt-tatiana
Copy link
Contributor Author

@qt-tatiana: Please do not forget to resolve fixed comments.

Done, thanks! :)

Copy link
Contributor

@5chmidti 5chmidti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be very nice to get in

Options
-------

.. option:: IncludeStyle
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not for this patch) Sounds like it's about time to move this to the top-level configuration instead of duplicated for every check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #113577.

@qt-tatiana
Copy link
Contributor Author

Hi all! Is there anything else expected from my side?

@carlosgalvezp
Copy link
Contributor

Perhaps this has been discussed before, but I think the part about "add qt module" should be added on a separate patch, since adding a brand-new tidy module is a rather big thing.

So this patch could be only about adding the modernize check, and a follow-up patch could add the qt module and alias.

@qt-tatiana
Copy link
Contributor Author

Perhaps this has been discussed before, but I think the part about "add qt module" should be added on a separate patch, since adding a brand-new tidy module is a rather big thing.

So this patch could be only about adding the modernize check, and a follow-up patch could add the qt module and alias.

Done, I've deleted qt related code from current code-version.

@qt-tatiana qt-tatiana requested a review from 5chmidti November 18, 2024 10:00
@qt-tatiana
Copy link
Contributor Author

Ping :)

@qt-tatiana
Copy link
Contributor Author

Ping :)

Copy link
Contributor

@5chmidti 5chmidti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check will currently not flag the use of literals like so:

void foo(unsigned short *uArray){
    uArray[0] <= 10;
}

https://godbolt.org/z/a6W9xGcMY

because they do not contain an implicit cast. Only one of the two operands needs to have an implicit cast to the other operands type for the comparison to be a potential issue.

Copy link
Contributor

@5chmidti 5chmidti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small things to address

Copy link
Contributor

@HerrCai0907 HerrCai0907 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your patience.
LGTM with some small nits.

@carlosgalvezp
Copy link
Contributor

Nit tip: the additional fixup! in the commit message for each additional commit is rather distracting, it would be best if each commit were be named after the changes it introduces w.r.t. the previous commit.

Copy link
Contributor

@5chmidti 5chmidti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@qt-tatiana
Copy link
Contributor Author

Nit tip: the additional fixup! in the commit message for each additional commit is rather distracting, it would be best if each commit were be named after the changes it introduces w.r.t. the previous commit.

Ok, thanks! Started to use it in my next commits

@qt-tatiana
Copy link
Contributor Author

Hi all,
Sorry for silly question, but what is next?
if I already have 2 approvals, should I squash all fix-ups and merge? (Btw, I don't have write permisions, not sure if I can land anything by myself)

@HerrCai0907
Copy link
Contributor

if I already have 2 approvals, should I squash all fix-ups and merge?

see https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted

I can help you to merge it. please feel free to ping me.

@HerrCai0907
Copy link
Contributor

I post a RFC about the cooperation between clang-tidy and common used libraries. Maybe it can be a more generic solution to this kind of modern checks.

- modernize-use-integer-sign-comparison - replace comparisons between
signed and unsigned integers with their safe C++20 ``std::cmp_*``
alternative, if available.
@qt-tatiana
Copy link
Contributor Author

if I already have 2 approvals, should I squash all fix-ups and merge?

see https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted

I can help you to merge it. please feel free to ping me.

Thank you for help!

I squashed all fixups into the main change (used this instruction https://llvm.org/docs/GitHub.html#landing-your-change)

Whould it be possible to land the chamge? :-)

@HerrCai0907
Copy link
Contributor

I squashed all fixups into the main change (used this instruction

actually github will do squash. please do not squash it manually in further pr.

@HerrCai0907 HerrCai0907 merged commit 8b63bfb into llvm:main Dec 11, 2024
7 of 8 checks passed
Copy link

@qt-tatiana Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

@qt-tatiana
Copy link
Contributor Author

I squashed all fixups into the main change (used this instruction

actually github will do squash. please do not squash it manually in further pr.

Ooops, I didn't know. Thank you!

@llvm-ci
Copy link
Collaborator

llvm-ci commented Dec 12, 2024

LLVM Buildbot has detected a new failure on builder premerge-monolithic-linux running on premerge-linux-1 while building clang-tools-extra at step 7 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/17264

Here is the relevant piece of the build log for the reference
Step 7 (test-build-unified-tree-check-all) failure: test (failure)
...
PASS: UBSan-Standalone-lld-x86_64 :: TestCases/Misc/abs.cpp (92858 of 96836)
PASS: UBSan-Standalone-lld-x86_64 :: TestCases/Misc/log-path_test.cpp (92859 of 96836)
PASS: UBSan-Standalone-lld-x86_64 :: TestCases/Misc/nullability.c (92860 of 96836)
PASS: UBSan-Standalone-lld-x86_64 :: TestCases/Integer/uincdec-overflow.cpp (92861 of 96836)
PASS: UBSan-Standalone-lld-x86_64 :: TestCases/ImplicitConversion/integer-sign-change-ignorelist.c (92862 of 96836)
PASS: UBSan-Standalone-lld-x86_64 :: TestCases/Misc/monitor.cpp (92863 of 96836)
PASS: UBSan-Standalone-lld-x86_64 :: TestCases/ImplicitConversion/integer-sign-change-incdec.c (92864 of 96836)
PASS: UBSan-Standalone-lld-x86_64 :: TestCases/Misc/print_summary.c (92865 of 96836)
PASS: UBSan-Standalone-lld-x86_64 :: TestCases/Misc/builtins.cpp (92866 of 96836)
TIMEOUT: MLIR :: Examples/standalone/test.toy (92867 of 96836)
******************** TEST 'MLIR :: Examples/standalone/test.toy' FAILED ********************
Exit Code: 1
Timeout: Reached timeout of 60 seconds

Command Output (stdout):
--
# RUN: at line 1
"/etc/cmake/bin/cmake" "/build/buildbot/premerge-monolithic-linux/llvm-project/mlir/examples/standalone" -G "Ninja"  -DCMAKE_CXX_COMPILER=/usr/bin/clang++  -DCMAKE_C_COMPILER=/usr/bin/clang   -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/build/buildbot/premerge-monolithic-linux/build/lib/cmake/mlir  -DLLVM_USE_LINKER=lld  -DPython3_EXECUTABLE="/usr/bin/python3.10"
# executed command: /etc/cmake/bin/cmake /build/buildbot/premerge-monolithic-linux/llvm-project/mlir/examples/standalone -G Ninja -DCMAKE_CXX_COMPILER=/usr/bin/clang++ -DCMAKE_C_COMPILER=/usr/bin/clang -DLLVM_ENABLE_LIBCXX=OFF -DMLIR_DIR=/build/buildbot/premerge-monolithic-linux/build/lib/cmake/mlir -DLLVM_USE_LINKER=lld -DPython3_EXECUTABLE=/usr/bin/python3.10
# .---command stdout------------
# | -- The CXX compiler identification is Clang 16.0.6
# | -- The C compiler identification is Clang 16.0.6
# | -- Detecting CXX compiler ABI info
# | -- Detecting CXX compiler ABI info - done
# | -- Check for working CXX compiler: /usr/bin/clang++ - skipped
# | -- Detecting CXX compile features
# | -- Detecting CXX compile features - done
# | -- Detecting C compiler ABI info
# | -- Detecting C compiler ABI info - done
# | -- Check for working C compiler: /usr/bin/clang - skipped
# | -- Detecting C compile features
# | -- Detecting C compile features - done
# | -- Looking for histedit.h
# | -- Looking for histedit.h - found
# | -- Found LibEdit: /usr/include (found version "2.11") 
# | -- Found ZLIB: /usr/lib/x86_64-linux-gnu/libz.so (found version "1.2.11") 
# | -- Found LibXml2: /usr/lib/x86_64-linux-gnu/libxml2.so (found version "2.9.13") 
# | -- Using MLIRConfig.cmake in: /build/buildbot/premerge-monolithic-linux/build/lib/cmake/mlir
# | -- Using LLVMConfig.cmake in: /build/buildbot/premerge-monolithic-linux/build/lib/cmake/llvm
# | -- Linker detection: unknown
# | -- Performing Test LLVM_LIBSTDCXX_MIN
# | -- Performing Test LLVM_LIBSTDCXX_MIN - Success
# | -- Performing Test LLVM_LIBSTDCXX_SOFT_ERROR
# | -- Performing Test LLVM_LIBSTDCXX_SOFT_ERROR - Success
# | -- Performing Test CXX_SUPPORTS_CUSTOM_LINKER
# | -- Performing Test CXX_SUPPORTS_CUSTOM_LINKER - Success
# | -- Performing Test C_SUPPORTS_FPIC
# | -- Performing Test C_SUPPORTS_FPIC - Success
# | -- Performing Test CXX_SUPPORTS_FPIC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants