From 5ece73a5b14e86172b900f4ae9d63d8fa1590d4a Mon Sep 17 00:00:00 2001 From: Piotr Zegar Date: Mon, 25 Dec 2023 16:18:45 +0000 Subject: [PATCH 1/7] [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 435cb1e3fbcff3..a8a23b045f80bb 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("bugprone-branch-clone"); CheckFactories.registerCheck( "bugprone-casting-through-void"); + CheckFactories.registerCheck( + "bugprone-chained-comparison"); CheckFactories.registerCheck( "bugprone-compare-pointer-to-member-virtual-function"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index 70e7fbc7ec0c14..1cd6fb207d7625 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 00000000000000..07f65d062e51cf --- /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 +#include + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +namespace { + +bool isExprAComparisonOperator(const Expr *E) { + if (const auto *Op = dyn_cast_or_null(E->IgnoreImplicit())) + return Op->isComparisonOp(); + if (const auto *Op = + dyn_cast_or_null(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 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 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(OpExpr), Output) || + extractData(dyn_cast_or_null(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("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 00000000000000..953ab63298df7a --- /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 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 6e7554e0433c25..8410ea7e14cda6 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 + ` 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 ` 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 00000000000000..fb704c1eec68b4 --- /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 39d8b490d927cc..bcfc68a04562f2 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 `, "Yes" :doc:`bugprone-branch-clone `, :doc:`bugprone-casting-through-void `, + :doc:`bugprone-chained-comparison `, :doc:`bugprone-compare-pointer-to-member-virtual-function `, :doc:`bugprone-copy-constructor-init `, "Yes" :doc:`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 00000000000000..dea1ee732aa01e --- /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 Date: Tue, 16 Jan 2024 19:47:35 +0000 Subject: [PATCH 2/7] 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 07f65d062e51cf..8d2844438157ab 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(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 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 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 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(OpExpr), Output) || - extractData(dyn_cast_or_null(OpExpr), Output); +void ChainedComparisonData::extract(const Expr *Op) { + extract(dyn_cast_or_null(Op)) || + extract(dyn_cast_or_null(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("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 dea1ee732aa01e..8a664aa205acf4 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 Date: Thu, 18 Jan 2024 18:10:57 +0000 Subject: [PATCH 3/7] 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 953ab63298df7a..a914149a42e69f 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 getCheckTraversalKind() const override { return TK_IgnoreUnlessSpelledInSource; } diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 8410ea7e14cda6..b9803edf02cabc 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -140,7 +140,7 @@ New checks ` 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 ` 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 fb704c1eec68b4..45b069a8e29de1 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 00000000000000..53c6139dcfbc21 --- /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; +} + From 154587f432523f1e58857d639c5828505e7fe13a Mon Sep 17 00:00:00 2001 From: Piotr Zegar Date: Sun, 21 Jan 2024 22:01:05 +0000 Subject: [PATCH 4/7] Refactor extract to use ifs --- .../bugprone/ChainedComparisonCheck.cpp | 47 ++++++++++--------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp index 8d2844438157ab..510c1fec52bfcb 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp @@ -12,7 +12,6 @@ #include "llvm/ADT/SmallString.h" #include "llvm/ADT/SmallVector.h" #include -#include using namespace clang::ast_matchers; @@ -49,8 +48,8 @@ struct ChainedComparisonData { 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); + void extract(const BinaryOperator *Op); + void extract(const CXXOperatorCallExpr *Op); }; void ChainedComparisonData::add(const Expr *Operand) { @@ -63,31 +62,26 @@ void ChainedComparisonData::add(const Expr *Operand) { void ChainedComparisonData::add(llvm::StringRef Opcode) { Name += ' '; - Name += Opcode.str(); + Name += Opcode; } -bool ChainedComparisonData::extract(const BinaryOperator *Op) { - if (!Op) - return false; - - if (isExprAComparisonOperator(Op->getLHS())) - extract(Op->getLHS()->IgnoreImplicit()); +void ChainedComparisonData::extract(const BinaryOperator *Op) { + const Expr* LHS = Op->getLHS(); + if (isExprAComparisonOperator(LHS)) + extract(LHS->IgnoreImplicit()); else - add(Op->getLHS()->IgnoreUnlessSpelledInSource()); + add(LHS->IgnoreUnlessSpelledInSource()); add(Op->getOpcodeStr()); - if (isExprAComparisonOperator(Op->getRHS())) - extract(Op->getRHS()->IgnoreImplicit()); + const Expr* RHS = Op->getRHS(); + if (isExprAComparisonOperator(RHS)) + extract(RHS->IgnoreImplicit()); else - add(Op->getRHS()->IgnoreUnlessSpelledInSource()); - return true; + add(RHS->IgnoreUnlessSpelledInSource()); } -bool ChainedComparisonData::extract(const CXXOperatorCallExpr *Op) { - if (!Op || Op->getNumArgs() != 2U) - return false; - +void ChainedComparisonData::extract(const CXXOperatorCallExpr *Op) { const Expr *FirstArg = Op->getArg(0U)->IgnoreImplicit(); if (isExprAComparisonOperator(FirstArg)) extract(FirstArg); @@ -101,12 +95,21 @@ bool ChainedComparisonData::extract(const CXXOperatorCallExpr *Op) { extract(SecondArg); else add(SecondArg->IgnoreUnlessSpelledInSource()); - return true; } void ChainedComparisonData::extract(const Expr *Op) { - extract(dyn_cast_or_null(Op)) || - extract(dyn_cast_or_null(Op)); + if (!Op) + return; + + if (const auto* BinaryOp = dyn_cast(Op)) { + extract(BinaryOp); + return; + } + + if (const auto* OverloadedOp = dyn_cast(Op)) { + if (OverloadedOp->getNumArgs() == 2U) + extract(OverloadedOp); + } } } // namespace From 3872f3c40755b76d6b5c027451925c3e585f12e3 Mon Sep 17 00:00:00 2001 From: Piotr Zegar Date: Sun, 21 Jan 2024 22:15:13 +0000 Subject: [PATCH 5/7] Added Source range to diangostic --- .../clang-tidy/bugprone/ChainedComparisonCheck.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp index 510c1fec52bfcb..82e66348c10cb1 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp @@ -138,12 +138,12 @@ void ChainedComparisonCheck::check(const MatchFinder::MatchResult &Result) { "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(); + << llvm::StringRef(Data.Name).trim() << MatchedOperator->getSourceRange(); for (std::size_t Index = 0U; Index < Data.Operands.size(); ++Index) { diag(Data.Operands[Index]->getBeginLoc(), "operand 'v%0' is here", DiagnosticIDs::Note) - << Index; + << Index << Data.Operands[Index]->getSourceRange(); } } From 9da19e1e90afd21ef6c783159265dac234846b8b Mon Sep 17 00:00:00 2001 From: Piotr Zegar Date: Sun, 21 Jan 2024 22:47:30 +0000 Subject: [PATCH 6/7] Rework implicit drop --- .../bugprone/ChainedComparisonCheck.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp index 82e66348c10cb1..475de8a5a50a37 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp @@ -66,19 +66,19 @@ void ChainedComparisonData::add(llvm::StringRef Opcode) { } void ChainedComparisonData::extract(const BinaryOperator *Op) { - const Expr* LHS = Op->getLHS(); + const Expr *LHS = Op->getLHS()->IgnoreImplicit(); if (isExprAComparisonOperator(LHS)) - extract(LHS->IgnoreImplicit()); + extract(LHS); else - add(LHS->IgnoreUnlessSpelledInSource()); + add(LHS); add(Op->getOpcodeStr()); - const Expr* RHS = Op->getRHS(); + const Expr *RHS = Op->getRHS()->IgnoreImplicit(); if (isExprAComparisonOperator(RHS)) - extract(RHS->IgnoreImplicit()); + extract(RHS); else - add(RHS->IgnoreUnlessSpelledInSource()); + add(RHS); } void ChainedComparisonData::extract(const CXXOperatorCallExpr *Op) { @@ -86,7 +86,7 @@ void ChainedComparisonData::extract(const CXXOperatorCallExpr *Op) { if (isExprAComparisonOperator(FirstArg)) extract(FirstArg); else - add(FirstArg->IgnoreUnlessSpelledInSource()); + add(FirstArg); add(getOperatorSpelling(Op->getOperator())); @@ -94,7 +94,7 @@ void ChainedComparisonData::extract(const CXXOperatorCallExpr *Op) { if (isExprAComparisonOperator(SecondArg)) extract(SecondArg); else - add(SecondArg->IgnoreUnlessSpelledInSource()); + add(SecondArg); } void ChainedComparisonData::extract(const Expr *Op) { From 34a9eb9106c8f224637fd54f836c1b41d922b923 Mon Sep 17 00:00:00 2001 From: Piotr Zegar Date: Mon, 22 Jan 2024 07:20:34 +0000 Subject: [PATCH 7/7] Update formating --- .../clang-tidy/bugprone/ChainedComparisonCheck.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp index 475de8a5a50a37..c6bb5bdf5ce917 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ChainedComparisonCheck.cpp @@ -101,12 +101,12 @@ void ChainedComparisonData::extract(const Expr *Op) { if (!Op) return; - if (const auto* BinaryOp = dyn_cast(Op)) { + if (const auto *BinaryOp = dyn_cast(Op)) { extract(BinaryOp); return; } - if (const auto* OverloadedOp = dyn_cast(Op)) { + if (const auto *OverloadedOp = dyn_cast(Op)) { if (OverloadedOp->getNumArgs() == 2U) extract(OverloadedOp); }