[clang-tools-extra] [clang-tidy] Add bugprone-chained-comparison check (PR #76365)
Piotr Zegar via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 18 10:14:07 PST 2024
https://github.com/PiotrZSL updated https://github.com/llvm/llvm-project/pull/76365
>From 5ece73a5b14e86172b900f4ae9d63d8fa1590d4a Mon Sep 17 00:00:00 2001
From: Piotr Zegar <piotr.zegar at nokia.com>
Date: Mon, 25 Dec 2023 16:18:45 +0000
Subject: [PATCH 1/3] [clang-tidy] Add bugprone-chained-comparison check
Check that flags chained comparison expressions,
such as a < b < c or a == b == c, which may have
unintended behavior due to implicit operator
associativity.
---
.../bugprone/BugproneTidyModule.cpp | 3 +
.../clang-tidy/bugprone/CMakeLists.txt | 1 +
.../bugprone/ChainedComparisonCheck.cpp | 161 ++++++++++++++++++
.../bugprone/ChainedComparisonCheck.h | 37 ++++
clang-tools-extra/docs/ReleaseNotes.rst | 6 +
.../checks/bugprone/chained-comparison.rst | 73 ++++++++
.../docs/clang-tidy/checks/list.rst | 1 +
.../checkers/bugprone/chained-comparison.cpp | 91 ++++++++++
8 files changed, 373 insertions(+)
create mode 100644 clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp
create mode 100644 clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h
create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/chained-comparison.rst
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 435cb1e3fbcff34..a8a23b045f80bb8 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -17,6 +17,7 @@
#include "BoolPointerImplicitConversionCheck.h"
#include "BranchCloneCheck.h"
#include "CastingThroughVoidCheck.h"
+#include "ChainedComparisonCheck.h"
#include "ComparePointerToMemberVirtualFunctionCheck.h"
#include "CopyConstructorInitCheck.h"
#include "DanglingHandleCheck.h"
@@ -108,6 +109,8 @@ class BugproneModule : public ClangTidyModule {
CheckFactories.registerCheck<BranchCloneCheck>("bugprone-branch-clone");
CheckFactories.registerCheck<CastingThroughVoidCheck>(
"bugprone-casting-through-void");
+ CheckFactories.registerCheck<ChainedComparisonCheck>(
+ "bugprone-chained-comparison");
CheckFactories.registerCheck<ComparePointerToMemberVirtualFunctionCheck>(
"bugprone-compare-pointer-to-member-virtual-function");
CheckFactories.registerCheck<CopyConstructorInitCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index 70e7fbc7ec0c144..1cd6fb207d7625a 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -12,6 +12,7 @@ add_clang_library(clangTidyBugproneModule
BranchCloneCheck.cpp
BugproneTidyModule.cpp
CastingThroughVoidCheck.cpp
+ ChainedComparisonCheck.cpp
ComparePointerToMemberVirtualFunctionCheck.cpp
CopyConstructorInitCheck.cpp
DanglingHandleCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp
new file mode 100644
index 000000000000000..07f65d062e51cfd
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp
@@ -0,0 +1,161 @@
+//===--- ChainedComparisonCheck.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 "ChainedComparisonCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
+#include <algorithm>
+#include <array>
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+namespace {
+
+bool isExprAComparisonOperator(const Expr *E) {
+ if (const auto *Op = dyn_cast_or_null<BinaryOperator>(E->IgnoreImplicit()))
+ return Op->isComparisonOp();
+ if (const auto *Op =
+ dyn_cast_or_null<CXXOperatorCallExpr>(E->IgnoreImplicit()))
+ return Op->isComparisonOp();
+ return false;
+}
+
+AST_MATCHER(BinaryOperator,
+ hasBinaryOperatorAChildComparisonOperatorWithoutParen) {
+ return isExprAComparisonOperator(Node.getLHS()) ||
+ isExprAComparisonOperator(Node.getRHS());
+}
+
+AST_MATCHER(CXXOperatorCallExpr,
+ hasCppOperatorAChildComparisonOperatorWithoutParen) {
+ return std::any_of(Node.arg_begin(), Node.arg_end(),
+ isExprAComparisonOperator);
+}
+
+constexpr std::array<llvm::StringRef, 26U> Letters = {
+ "a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m",
+ "n", "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z"};
+
+struct ChainedComparisonData {
+ llvm::SmallString<256U> Name;
+ llvm::SmallVector<const Expr *, 26U> Operands;
+ bool Full = false;
+
+ void Add(const Expr *Operand) {
+ if (Full)
+ return;
+ if (!Name.empty())
+ Name += ' ';
+ Name += Letters[Operands.size()];
+ Operands.push_back(Operand);
+
+ if (Operands.size() == Letters.size()) {
+ Name += " ...";
+ Full = true;
+ }
+ }
+
+ void Add(llvm::StringRef Opcode) {
+ if (Full)
+ return;
+
+ Name += ' ';
+ Name += Opcode.str();
+ }
+};
+
+} // namespace
+
+static void extractData(const Expr *Op, ChainedComparisonData &Output);
+
+inline bool extractData(const BinaryOperator *Op,
+ ChainedComparisonData &Output) {
+ if (!Op)
+ return false;
+
+ if (isExprAComparisonOperator(Op->getLHS()))
+ extractData(Op->getLHS()->IgnoreImplicit(), Output);
+ else
+ Output.Add(Op->getLHS()->IgnoreUnlessSpelledInSource());
+
+ Output.Add(Op->getOpcodeStr());
+
+ if (isExprAComparisonOperator(Op->getRHS()))
+ extractData(Op->getRHS()->IgnoreImplicit(), Output);
+ else
+ Output.Add(Op->getRHS()->IgnoreUnlessSpelledInSource());
+ return true;
+}
+
+inline bool extractData(const CXXOperatorCallExpr *Op,
+ ChainedComparisonData &Output) {
+ if (!Op || Op->getNumArgs() != 2U)
+ return false;
+
+ const Expr *FirstArg = Op->getArg(0U)->IgnoreImplicit();
+ if (isExprAComparisonOperator(FirstArg))
+ extractData(FirstArg, Output);
+ else
+ Output.Add(FirstArg->IgnoreUnlessSpelledInSource());
+
+ Output.Add(getOperatorSpelling(Op->getOperator()));
+
+ const Expr *SecondArg = Op->getArg(1U)->IgnoreImplicit();
+ if (isExprAComparisonOperator(SecondArg))
+ extractData(SecondArg, Output);
+ else
+ Output.Add(SecondArg->IgnoreUnlessSpelledInSource());
+ return true;
+}
+
+static void extractData(const Expr *OpExpr, ChainedComparisonData &Output) {
+ OpExpr->dump();
+ extractData(dyn_cast_or_null<BinaryOperator>(OpExpr), Output) ||
+ extractData(dyn_cast_or_null<CXXOperatorCallExpr>(OpExpr), Output);
+}
+
+void ChainedComparisonCheck::registerMatchers(MatchFinder *Finder) {
+ const auto OperatorMatcher = expr(anyOf(
+ binaryOperator(isComparisonOperator(),
+ hasBinaryOperatorAChildComparisonOperatorWithoutParen()),
+ cxxOperatorCallExpr(
+ isComparisonOperator(),
+ hasCppOperatorAChildComparisonOperatorWithoutParen())));
+
+ Finder->addMatcher(
+ expr(OperatorMatcher, unless(hasParent(OperatorMatcher))).bind("op"),
+ this);
+}
+
+void ChainedComparisonCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *MatchedOperator = Result.Nodes.getNodeAs<Expr>("op");
+
+ ChainedComparisonData Data;
+ extractData(MatchedOperator, Data);
+
+ if (Data.Operands.empty())
+ return;
+
+ diag(MatchedOperator->getBeginLoc(),
+ "chained comparison '%0' may generate unintended results, use "
+ "parentheses to specify order of evaluation or a logical operator to "
+ "separate comparison expressions")
+ << llvm::StringRef(Data.Name).trim();
+
+ for (std::size_t Index = 0U; Index < Data.Operands.size(); ++Index) {
+ diag(Data.Operands[Index]->getBeginLoc(), "operand '%0' is here",
+ DiagnosticIDs::Note)
+ << Letters[Index];
+ }
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h
new file mode 100644
index 000000000000000..953ab63298df7ab
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h
@@ -0,0 +1,37 @@
+//===--- ChainedComparisonCheck.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_BUGPRONE_CHAINEDCOMPARISONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CHAINEDCOMPARISONCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Check detects chained comparison operators that can lead to unintended
+/// behavior or logical errors in C++ code.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/chained-comparison.html
+class ChainedComparisonCheck : public ClangTidyCheck {
+public:
+ ChainedComparisonCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ 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;
+ }
+ std::optional<TraversalKind> getCheckTraversalKind() const override {
+ return TK_IgnoreUnlessSpelledInSource;
+ }
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CHAINEDCOMPARISONCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6e7554e0433c252..8410ea7e14cda61 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -136,6 +136,12 @@ New checks
Detects unsafe or redundant two-step casting operations involving ``void*``.
+- New :doc:`bugprone-chained-comparison
+ <clang-tidy/checks/bugprone/chained-comparison>` check.
+
+ Check detects chained comparison operators that can lead to unintended
+ behavior or logical errors in C++ code.
+
- New :doc:`bugprone-compare-pointer-to-member-virtual-function
<clang-tidy/checks/bugprone/compare-pointer-to-member-virtual-function>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/chained-comparison.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/chained-comparison.rst
new file mode 100644
index 000000000000000..fb704c1eec68b4f
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/chained-comparison.rst
@@ -0,0 +1,73 @@
+.. title:: clang-tidy - bugprone-chained-comparison
+
+bugprone-chained-comparison
+===========================
+
+Check detects chained comparison operators that can lead to unintended
+behavior or logical errors in C++ code.
+
+Chained comparisons are expressions that use multiple comparison operators
+to compare three or more values. For example, the expression ``a < b < c``
+compares the values of ``a``, ``b``, and ``c``. However, this expression does
+not evaluate as ``(a < b) && (b < c)``, which is probably what the developer
+intended. Instead, it evaluates as ``(a < b) < c``, which may produce
+unintended results, especially when the types of ``a``, ``b``, and ``c`` are
+different.
+
+To avoid such errors, the check will issue a warning when a chained
+comparison operator is detected, suggesting to use parentheses to specify
+the order of evaluation or to use a logical operator to separate comparison
+expressions.
+
+Consider the following examples:
+
+.. code-block:: c++
+
+ int a = 2, b = 6, c = 4;
+ if (a < b < c) {
+ // This block will be executed
+ }
+
+
+In this example, the developer intended to check if ``a`` is less than ``b``
+and ``b`` is less than ``c``. However, the expression ``a < b < c`` is
+equivalent to ``(a < b) < c``. Since ``a < b`` is ``true``, the expression
+``(a < b) < c`` is evaluated as ``1 < c``, which is equivalent to ``true < c``
+and is invalid in this case as ``b < c`` is ``false``.
+
+Even that above issue could be detected as comparison of ``int`` to ``bool``,
+there is more dangerous example:
+
+.. code-block:: c++
+
+ bool a = false, b = false, c = true;
+ if (a == b == c) {
+ // This block will be executed
+ }
+
+In this example, the developer intended to check if ``a``, ``b``, and ``c`` are
+all equal. However, the expression ``a == b == c`` is evaluated as
+``(a == b) == c``. Since ``a == b`` is true, the expression ``(a == b) == c``
+is evaluated as ``true == c``, which is equivalent to ``true == true``.
+This comparison yields ``true``, even though ``a`` and ``b`` are ``false``, and
+are not equal to ``c``.
+
+To avoid this issue, the developer can use a logical operator to separate the
+comparison expressions, like this:
+
+.. code-block:: c++
+
+ if (a == b && b == c) {
+ // This block will not be executed
+ }
+
+
+Alternatively, use of parentheses in the comparison expressions can make the
+developer's intention more explicit and help avoid misunderstanding.
+
+.. code-block:: c++
+
+ if ((a == b) == c) {
+ // This block will be executed
+ }
+
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 39d8b490d927cc1..bcfc68a04562f28 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -83,6 +83,7 @@ Clang-Tidy Checks
:doc:`bugprone-bool-pointer-implicit-conversion <bugprone/bool-pointer-implicit-conversion>`, "Yes"
:doc:`bugprone-branch-clone <bugprone/branch-clone>`,
:doc:`bugprone-casting-through-void <bugprone/casting-through-void>`,
+ :doc:`bugprone-chained-comparison <bugprone/chained-comparison>`,
:doc:`bugprone-compare-pointer-to-member-virtual-function <bugprone/compare-pointer-to-member-virtual-function>`,
:doc:`bugprone-copy-constructor-init <bugprone/copy-constructor-init>`, "Yes"
:doc:`bugprone-dangling-handle <bugprone/dangling-handle>`,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp
new file mode 100644
index 000000000000000..dea1ee732aa01ec
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp
@@ -0,0 +1,91 @@
+// RUN: %check_clang_tidy -std=c++98-or-later %s bugprone-chained-comparison %t
+
+void badly_chained_1(int x, int y, int z)
+{
+ bool result = x < y < z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a < b < c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_2(int x, int y, int z)
+{
+ bool result = x <= y <= z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a <= b <= c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_3(int x, int y, int z)
+{
+ bool result = x > y > z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a > b > c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_4(int x, int y, int z)
+{
+ bool result = x >= y >= z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a >= b >= c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_5(int x, int y, int z)
+{
+ bool result = x == y != z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a == b != c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_6(bool x, bool y, bool z)
+{
+ bool result = x != y == z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a != b == c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_multiple(bool a, bool b, bool c, bool d, bool e, bool f, bool g, bool h)
+{
+ bool result = a == b == c == d == e == f == g == h;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a == b == c == d == e == f == g == h' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_limit(bool v[29])
+{
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: chained comparison 'a == b == c == d == e == f == g == h == i == j == k == l == m == n == o == p == q == r == s == t == u == v == w == x == y == z ...' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+ bool result = v[0] == v[1] == v[2] == v[3] == v[4] == v[5] == v[6] == v[7] ==
+ v[8] == v[9] == v[10] == v[11] == v[12] == v[13] == v[14] ==
+ v[15] == v[16] == v[17] == v[18] == v[19] == v[20] == v[21] ==
+ v[22] == v[23] == v[24] == v[25] == v[26] == v[27] == v[28];
+
+}
+
+void badly_chained_parens2(int x, int y, int z, int t, int a, int b)
+{
+ bool result = (x < y) < (z && t) > (a == b);
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a < b > c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_inner(int x, int y, int z, int t, int u)
+{
+ bool result = x && y < z < t && u;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: chained comparison 'a < b < c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void properly_chained_1(int x, int y, int z)
+{
+ bool result = x < y && y < z;
+}
+
+void properly_chained_2(int x, int y, bool z)
+{
+ bool result = (x < y) == z;
+}
+
+struct Value {
+ bool operator<(const Value&) const;
+};
+
+bool operator==(bool, const Value&);
+
+bool badWithCppOperator(Value a, Value b, Value c) {
+ return a < b == c;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: chained comparison 'a < b == c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+bool mixedBinaryAndCpp(Value a, Value b, bool c) {
+ return a < b == c;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: chained comparison 'a < b == c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
>From 7c02aa0c1ca8628f2e52d0ebd74069f0f39120d5 Mon Sep 17 00:00:00 2001
From: Piotr Zegar <piotr.zegar at nokia.com>
Date: Tue, 16 Jan 2024 19:47:35 +0000
Subject: [PATCH 2/3] Resolve review comments
---
.../bugprone/ChainedComparisonCheck.cpp | 96 ++++++++-----------
.../checkers/bugprone/chained-comparison.cpp | 24 ++---
2 files changed, 53 insertions(+), 67 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp
index 07f65d062e51cfd..8d2844438157abf 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp
@@ -17,10 +17,7 @@
using namespace clang::ast_matchers;
namespace clang::tidy::bugprone {
-
-namespace {
-
-bool isExprAComparisonOperator(const Expr *E) {
+static bool isExprAComparisonOperator(const Expr *E) {
if (const auto *Op = dyn_cast_or_null<BinaryOperator>(E->IgnoreImplicit()))
return Op->isComparisonOp();
if (const auto *Op =
@@ -29,6 +26,7 @@ bool isExprAComparisonOperator(const Expr *E) {
return false;
}
+namespace {
AST_MATCHER(BinaryOperator,
hasBinaryOperatorAChildComparisonOperatorWithoutParen) {
return isExprAComparisonOperator(Node.getLHS()) ||
@@ -41,88 +39,78 @@ AST_MATCHER(CXXOperatorCallExpr,
isExprAComparisonOperator);
}
-constexpr std::array<llvm::StringRef, 26U> Letters = {
- "a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m",
- "n", "o", "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z"};
-
struct ChainedComparisonData {
llvm::SmallString<256U> Name;
- llvm::SmallVector<const Expr *, 26U> Operands;
- bool Full = false;
-
- void Add(const Expr *Operand) {
- if (Full)
- return;
- if (!Name.empty())
- Name += ' ';
- Name += Letters[Operands.size()];
- Operands.push_back(Operand);
-
- if (Operands.size() == Letters.size()) {
- Name += " ...";
- Full = true;
- }
- }
+ llvm::SmallVector<const Expr *, 32U> Operands;
- void Add(llvm::StringRef Opcode) {
- if (Full)
- return;
+ explicit ChainedComparisonData(const Expr *Op) { extract(Op); }
- Name += ' ';
- Name += Opcode.str();
- }
+private:
+ void add(const Expr *Operand);
+ void add(llvm::StringRef Opcode);
+ void extract(const Expr *Op);
+ bool extract(const BinaryOperator *Op);
+ bool extract(const CXXOperatorCallExpr *Op);
};
-} // namespace
+void ChainedComparisonData::add(const Expr *Operand) {
+ if (!Name.empty())
+ Name += ' ';
+ Name += 'v';
+ Name += std::to_string(Operands.size());
+ Operands.push_back(Operand);
+}
-static void extractData(const Expr *Op, ChainedComparisonData &Output);
+void ChainedComparisonData::add(llvm::StringRef Opcode) {
+ Name += ' ';
+ Name += Opcode.str();
+}
-inline bool extractData(const BinaryOperator *Op,
- ChainedComparisonData &Output) {
+bool ChainedComparisonData::extract(const BinaryOperator *Op) {
if (!Op)
return false;
if (isExprAComparisonOperator(Op->getLHS()))
- extractData(Op->getLHS()->IgnoreImplicit(), Output);
+ extract(Op->getLHS()->IgnoreImplicit());
else
- Output.Add(Op->getLHS()->IgnoreUnlessSpelledInSource());
+ add(Op->getLHS()->IgnoreUnlessSpelledInSource());
- Output.Add(Op->getOpcodeStr());
+ add(Op->getOpcodeStr());
if (isExprAComparisonOperator(Op->getRHS()))
- extractData(Op->getRHS()->IgnoreImplicit(), Output);
+ extract(Op->getRHS()->IgnoreImplicit());
else
- Output.Add(Op->getRHS()->IgnoreUnlessSpelledInSource());
+ add(Op->getRHS()->IgnoreUnlessSpelledInSource());
return true;
}
-inline bool extractData(const CXXOperatorCallExpr *Op,
- ChainedComparisonData &Output) {
+bool ChainedComparisonData::extract(const CXXOperatorCallExpr *Op) {
if (!Op || Op->getNumArgs() != 2U)
return false;
const Expr *FirstArg = Op->getArg(0U)->IgnoreImplicit();
if (isExprAComparisonOperator(FirstArg))
- extractData(FirstArg, Output);
+ extract(FirstArg);
else
- Output.Add(FirstArg->IgnoreUnlessSpelledInSource());
+ add(FirstArg->IgnoreUnlessSpelledInSource());
- Output.Add(getOperatorSpelling(Op->getOperator()));
+ add(getOperatorSpelling(Op->getOperator()));
const Expr *SecondArg = Op->getArg(1U)->IgnoreImplicit();
if (isExprAComparisonOperator(SecondArg))
- extractData(SecondArg, Output);
+ extract(SecondArg);
else
- Output.Add(SecondArg->IgnoreUnlessSpelledInSource());
+ add(SecondArg->IgnoreUnlessSpelledInSource());
return true;
}
-static void extractData(const Expr *OpExpr, ChainedComparisonData &Output) {
- OpExpr->dump();
- extractData(dyn_cast_or_null<BinaryOperator>(OpExpr), Output) ||
- extractData(dyn_cast_or_null<CXXOperatorCallExpr>(OpExpr), Output);
+void ChainedComparisonData::extract(const Expr *Op) {
+ extract(dyn_cast_or_null<BinaryOperator>(Op)) ||
+ extract(dyn_cast_or_null<CXXOperatorCallExpr>(Op));
}
+} // namespace
+
void ChainedComparisonCheck::registerMatchers(MatchFinder *Finder) {
const auto OperatorMatcher = expr(anyOf(
binaryOperator(isComparisonOperator(),
@@ -139,9 +127,7 @@ void ChainedComparisonCheck::registerMatchers(MatchFinder *Finder) {
void ChainedComparisonCheck::check(const MatchFinder::MatchResult &Result) {
const auto *MatchedOperator = Result.Nodes.getNodeAs<Expr>("op");
- ChainedComparisonData Data;
- extractData(MatchedOperator, Data);
-
+ ChainedComparisonData Data(MatchedOperator);
if (Data.Operands.empty())
return;
@@ -152,9 +138,9 @@ void ChainedComparisonCheck::check(const MatchFinder::MatchResult &Result) {
<< llvm::StringRef(Data.Name).trim();
for (std::size_t Index = 0U; Index < Data.Operands.size(); ++Index) {
- diag(Data.Operands[Index]->getBeginLoc(), "operand '%0' is here",
+ diag(Data.Operands[Index]->getBeginLoc(), "operand 'v%0' is here",
DiagnosticIDs::Note)
- << Letters[Index];
+ << Index;
}
}
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp
index dea1ee732aa01ec..8a664aa205acf48 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.cpp
@@ -4,47 +4,47 @@ void badly_chained_1(int x, int y, int z)
{
bool result = x < y < z;
}
-// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a < b < c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'v0 < v1 < v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
void badly_chained_2(int x, int y, int z)
{
bool result = x <= y <= z;
}
-// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a <= b <= c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'v0 <= v1 <= v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
void badly_chained_3(int x, int y, int z)
{
bool result = x > y > z;
}
-// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a > b > c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'v0 > v1 > v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
void badly_chained_4(int x, int y, int z)
{
bool result = x >= y >= z;
}
-// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a >= b >= c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'v0 >= v1 >= v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
void badly_chained_5(int x, int y, int z)
{
bool result = x == y != z;
}
-// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a == b != c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'v0 == v1 != v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
void badly_chained_6(bool x, bool y, bool z)
{
bool result = x != y == z;
}
-// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a != b == c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'v0 != v1 == v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
void badly_chained_multiple(bool a, bool b, bool c, bool d, bool e, bool f, bool g, bool h)
{
bool result = a == b == c == d == e == f == g == h;
}
-// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a == b == c == d == e == f == g == h' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'v0 == v1 == v2 == v3 == v4 == v5 == v6 == v7' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
void badly_chained_limit(bool v[29])
{
-// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: chained comparison 'a == b == c == d == e == f == g == h == i == j == k == l == m == n == o == p == q == r == s == t == u == v == w == x == y == z ...' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+// CHECK-MESSAGES: :[[@LINE+1]]:19: warning: chained comparison 'v0 == v1 == v2 == v3 == v4 == v5 == v6 == v7 == v8 == v9 == v10 == v11 == v12 == v13 == v14 == v15 == v16 == v17 == v18 == v19 == v20 == v21 == v22 == v23 == v24 == v25 == v26 == v27 == v28' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
bool result = v[0] == v[1] == v[2] == v[3] == v[4] == v[5] == v[6] == v[7] ==
v[8] == v[9] == v[10] == v[11] == v[12] == v[13] == v[14] ==
v[15] == v[16] == v[17] == v[18] == v[19] == v[20] == v[21] ==
@@ -56,13 +56,13 @@ void badly_chained_parens2(int x, int y, int z, int t, int a, int b)
{
bool result = (x < y) < (z && t) > (a == b);
}
-// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'a < b > c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: chained comparison 'v0 < v1 > v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
void badly_chained_inner(int x, int y, int z, int t, int u)
{
bool result = x && y < z < t && u;
}
-// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: chained comparison 'a < b < c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+// CHECK-MESSAGES: :[[@LINE-2]]:24: warning: chained comparison 'v0 < v1 < v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
void properly_chained_1(int x, int y, int z)
{
@@ -83,9 +83,9 @@ bool operator==(bool, const Value&);
bool badWithCppOperator(Value a, Value b, Value c) {
return a < b == c;
}
-// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: chained comparison 'a < b == c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: chained comparison 'v0 < v1 == v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
bool mixedBinaryAndCpp(Value a, Value b, bool c) {
return a < b == c;
}
-// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: chained comparison 'a < b == c' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+// CHECK-MESSAGES: :[[@LINE-2]]:12: warning: chained comparison 'v0 < v1 == v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
>From 2eabc11aa504c303a916729f1111016475ebeb52 Mon Sep 17 00:00:00 2001
From: Piotr Zegar <piotr.zegar at nokia.com>
Date: Thu, 18 Jan 2024 18:10:57 +0000
Subject: [PATCH 3/3] Enable check in C
---
.../bugprone/ChainedComparisonCheck.h | 5 +-
clang-tools-extra/docs/ReleaseNotes.rst | 2 +-
.../checks/bugprone/chained-comparison.rst | 2 +-
.../checkers/bugprone/chained-comparison.c | 76 +++++++++++++++++++
4 files changed, 79 insertions(+), 6 deletions(-)
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.c
diff --git a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h
index 953ab63298df7ab..a914149a42e69f0 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.h
@@ -14,7 +14,7 @@
namespace clang::tidy::bugprone {
/// Check detects chained comparison operators that can lead to unintended
-/// behavior or logical errors in C++ code.
+/// behavior or logical errors.
///
/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/chained-comparison.html
@@ -24,9 +24,6 @@ class ChainedComparisonCheck : public ClangTidyCheck {
: ClangTidyCheck(Name, Context) {}
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;
- }
std::optional<TraversalKind> getCheckTraversalKind() const override {
return TK_IgnoreUnlessSpelledInSource;
}
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 8410ea7e14cda61..b9803edf02cabc2 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -140,7 +140,7 @@ New checks
<clang-tidy/checks/bugprone/chained-comparison>` check.
Check detects chained comparison operators that can lead to unintended
- behavior or logical errors in C++ code.
+ behavior or logical errors.
- New :doc:`bugprone-compare-pointer-to-member-virtual-function
<clang-tidy/checks/bugprone/compare-pointer-to-member-virtual-function>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/chained-comparison.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/chained-comparison.rst
index fb704c1eec68b4f..45b069a8e29de11 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/chained-comparison.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/chained-comparison.rst
@@ -4,7 +4,7 @@ bugprone-chained-comparison
===========================
Check detects chained comparison operators that can lead to unintended
-behavior or logical errors in C++ code.
+behavior or logical errors.
Chained comparisons are expressions that use multiple comparison operators
to compare three or more values. For example, the expression ``a < b < c``
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.c
new file mode 100644
index 000000000000000..53c6139dcfbc21d
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/chained-comparison.c
@@ -0,0 +1,76 @@
+// RUN: %check_clang_tidy %s bugprone-chained-comparison %t
+
+void badly_chained_1(int x, int y, int z)
+{
+ int result = x < y < z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 < v1 < v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_2(int x, int y, int z)
+{
+ int result = x <= y <= z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 <= v1 <= v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_3(int x, int y, int z)
+{
+ int result = x > y > z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 > v1 > v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_4(int x, int y, int z)
+{
+ int result = x >= y >= z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 >= v1 >= v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_5(int x, int y, int z)
+{
+ int result = x == y != z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 == v1 != v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_6(int x, int y, int z)
+{
+ int result = x != y == z;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 != v1 == v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_multiple(int a, int b, int c, int d, int e, int f, int g, int h)
+{
+ int result = a == b == c == d == e == f == g == h;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 == v1 == v2 == v3 == v4 == v5 == v6 == v7' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_limit(int v[29])
+{
+// CHECK-MESSAGES: :[[@LINE+1]]:18: warning: chained comparison 'v0 == v1 == v2 == v3 == v4 == v5 == v6 == v7 == v8 == v9 == v10 == v11 == v12 == v13 == v14 == v15 == v16 == v17 == v18 == v19 == v20 == v21 == v22 == v23 == v24 == v25 == v26 == v27 == v28' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+ int result = v[0] == v[1] == v[2] == v[3] == v[4] == v[5] == v[6] == v[7] ==
+ v[8] == v[9] == v[10] == v[11] == v[12] == v[13] == v[14] ==
+ v[15] == v[16] == v[17] == v[18] == v[19] == v[20] == v[21] ==
+ v[22] == v[23] == v[24] == v[25] == v[26] == v[27] == v[28];
+
+}
+
+void badly_chained_parens2(int x, int y, int z, int t, int a, int b)
+{
+ int result = (x < y) < (z && t) > (a == b);
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:18: warning: chained comparison 'v0 < v1 > v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void badly_chained_inner(int x, int y, int z, int t, int u)
+{
+ int result = x && y < z < t && u;
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:23: warning: chained comparison 'v0 < v1 < v2' may generate unintended results, use parentheses to specify order of evaluation or a logical operator to separate comparison expressions [bugprone-chained-comparison]
+
+void properly_chained_1(int x, int y, int z)
+{
+ int result = x < y && y < z;
+}
+
+void properly_chained_2(int x, int y, int z)
+{
+ int result = (x < y) == z;
+}
+
More information about the cfe-commits
mailing list