Skip to content
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

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,13 @@ UseIntegerSignComparisonCheck::UseIntegerSignComparisonCheck(
IncludeInserter(Options.getLocalOrGlobal("IncludeStyle",
qt-tatiana marked this conversation as resolved.
Show resolved Hide resolved
utils::IncludeSorter::IS_LLVM),
areDiagsSelfContained()),
IsQtApplication(Options.get("IsQtApplication", false)) {}
IsQtApplication(Options.get("IsQtApplication", false)),
StringsMatchHeader(Options.get("StringsMatchHeader", "<utility>")) {}

void UseIntegerSignComparisonCheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "IsQtApplication", IsQtApplication);
Options.store(Opts, "StringsMatchHeader", StringsMatchHeader);
}

void UseIntegerSignComparisonCheck::registerMatchers(MatchFinder *Finder) {
Expand Down Expand Up @@ -84,7 +86,7 @@ UseIntegerSignComparisonCheck::parseOpCode(BinaryOperator::Opcode code) const {
case BO_NE:
return std::string("cmp_not_equal");
default:
return std::string();
return {};
}
}

Expand All @@ -107,7 +109,7 @@ void UseIntegerSignComparisonCheck::check(
if (!SignedCastExpression->isValueDependent() &&
SignedCastExpression->getSubExpr()->EvaluateAsInt(EVResult,
*Result.Context)) {
llvm::APSInt SValue = EVResult.Val.getInt();
const llvm::APSInt SValue = EVResult.Val.getInt();
if (SValue.isNonNegative())
return;
}
Expand All @@ -117,18 +119,18 @@ void UseIntegerSignComparisonCheck::check(
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)
const BinaryOperator::Opcode OpCode = BinaryOp->getOpcode();
const auto *Lhs = BinaryOp->getLHS()->IgnoreParenImpCasts();
qt-tatiana marked this conversation as resolved.
Show resolved Hide resolved
const auto *Rhs = BinaryOp->getRHS()->IgnoreParenImpCasts();
if (Lhs == nullptr || Rhs == nullptr)
return;

StringRef LHSString(Lexer::getSourceText(
CharSourceRange::getTokenRange(LHS->getSourceRange()),
const StringRef LhsString(Lexer::getSourceText(
CharSourceRange::getTokenRange(Lhs->getSourceRange()),
*Result.SourceManager, getLangOpts()));

StringRef RHSString(Lexer::getSourceText(
CharSourceRange::getTokenRange(RHS->getSourceRange()),
const StringRef RhsString(Lexer::getSourceText(
CharSourceRange::getTokenRange(Rhs->getSourceRange()),
*Result.SourceManager, getLangOpts()));

DiagnosticBuilder Diag =
Expand All @@ -140,14 +142,11 @@ void UseIntegerSignComparisonCheck::check(
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);
}

Expand All @@ -157,13 +156,13 @@ void UseIntegerSignComparisonCheck::check(
CharSourceRange::getTokenRange(BinaryOp->getBeginLoc(),
BinaryOp->getEndLoc()),
StringRef(std::string(CmpNamespace) + std::string("(") +
std::string(LHSString) + std::string(", ") +
std::string(RHSString) + 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));
StringsMatchHeader);
}

} // namespace clang::tidy::modernize
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class UseIntegerSignComparisonCheck : public ClangTidyCheck {

utils::IncludeInserter IncludeInserter;
bool IsQtApplication = false;
const StringRef StringsMatchHeader;
};

} // namespace clang::tidy::modernize
Expand Down
4 changes: 2 additions & 2 deletions clang-tools-extra/clang-tidy/qt/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ add_clang_library(clangTidyQtModule
DEPENDS
omp_gen
ClangDriverOptions
)
)

clang_target_link_libraries(clangTidyQtModule
PRIVATE
Expand All @@ -23,4 +23,4 @@ clang_target_link_libraries(clangTidyQtModule
clangBasic
clangLex
clangTooling
)
)
8 changes: 5 additions & 3 deletions clang-tools-extra/clang-tidy/qt/QtTidyModule.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//===-- QtTidyModule.cpp - clang-tidy ----------------------===//
//===--- 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.
Expand Down Expand Up @@ -26,8 +26,10 @@ class QtClangTidyModule : public ClangTidyModule {
ClangTidyOptions Options;
ClangTidyOptions::OptionMap &Opts = Options.CheckOptions;

Opts["qt-integer-sign-comparison."
"IsQtApplication"] = "true";
Opts["qt-integer-sign-comparison.IncludeStyle"] = "llvm";
Opts["qt-integer-sign-comparison.IsQtApplication"] = "true";
Opts["qt-integer-sign-comparison.StringsMatchHeader"] =
"<QtCore/q20utility.h>";

return Options;
}
Expand Down
21 changes: 11 additions & 10 deletions clang-tools-extra/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -123,20 +123,20 @@ 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:`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
causes cross-compiler incompatibility.

- New :doc:`modernize-use-integer-sign-comparison
qt-tatiana marked this conversation as resolved.
Show resolved Hide resolved
<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.
integers comparisons to std::cmp_equal, std::cmp_not_equal,
qt-tatiana marked this conversation as resolved.
Show resolved Hide resolved
std::cmp_less, std::cmp_greater, std::cmp_less_equal and
std::cmp_greater_equal functions.

- 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
causes cross-compiler incompatibility.

New check aliases
^^^^^^^^^^^^^^^^^
Expand All @@ -147,10 +147,11 @@ New check aliases

- New alias :doc:`qt-integer-sign-comparison
<clang-tidy/checks/qt/integer-sign-comparison>` to
doc:`modernize-use-integer-sign-comparison
: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.
q20::cmp_equal, q20::cmp_not_equal, q20::cmp_less, q20::cmp_greater,
q20::cmp_less_equal and q20::cmp_greater_equal functions.
The check assumes the analysed code is Qt-based code.

Changes in existing checks
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
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.
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, std::cmp_not_equal,
std::cmp_less, std::cmp_greater, std::cmp_less_equal and
std::cmp_greater_equal functions.

Examples of fixes created by the check:

Expand Down Expand Up @@ -33,11 +36,3 @@ becomes

return 1;
}

Options
-------

.. option:: IncludeStyle

A string specifying which include-style is used, `llvm` or `google`. Default
is `llvm`.
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
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.
The qt-integer-sign-comparison check is an alias, please see
:doc:`modernize-use-integer-sign-comparison <../modernize/use-integer-sign-comparison>`
for more information.

Examples of fixes created by the check:

Expand Down Expand Up @@ -62,3 +62,7 @@ Options

When `true` (default `false`), then it is assumed that the code being analyzed
qt-tatiana marked this conversation as resolved.
Show resolved Hide resolved
is the Qt-based code.

.. option:: StringsMatchHeader
A string specifying a include header file to be added by fix-it. Default
Copy link
Contributor

Choose a reason for hiding this comment

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

Please separate with newline.

Copy link
Author

Choose a reason for hiding this comment

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

I updated my solution a little bit and deleted this property, but forgot to delete from doc. thanks!

is `<utility>`.
Loading