diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -15,6 +15,7 @@ #include "BadSignalToKillThreadCheck.h" #include "BoolPointerImplicitConversionCheck.h" #include "BranchCloneCheck.h" +#include "ComplexConditionsCheck.h" #include "CopyConstructorInitCheck.h" #include "DanglingHandleCheck.h" #include "DynamicStaticInitializersCheck.h" @@ -81,6 +82,8 @@ "bugprone-bool-pointer-implicit-conversion"); CheckFactories.registerCheck( "bugprone-branch-clone"); + CheckFactories.registerCheck( + "bugprone-complex-conditions"); CheckFactories.registerCheck( "bugprone-copy-constructor-init"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -10,6 +10,7 @@ BoolPointerImplicitConversionCheck.cpp BranchCloneCheck.cpp BugproneTidyModule.cpp + ComplexConditionsCheck.cpp CopyConstructorInitCheck.cpp DanglingHandleCheck.cpp DynamicStaticInitializersCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/ComplexConditionsCheck.h b/clang-tools-extra/clang-tidy/bugprone/ComplexConditionsCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/ComplexConditionsCheck.h @@ -0,0 +1,44 @@ +//===--- ComplexConditionsCheck.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_COMPLEXCONDITIONSCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_COMPLEXCONDITIONSCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// This checker finds cases where relational expressions have no meaning. +/// For example, (x <= y <= z) has no meaning, but just happens to parse to +/// something deterministic. (x <= y <= z) is parsed to ((x <= y) <= z). The +/// (x<=y) returns a boolean value which is promoted to an integral context, +/// 0 (for false) or 1 (for true). So when (x <= y) the expression becomes +/// (1 <= z) and when (x > y) the expression becomes (0 <= z). When asked to +/// give all warnings, gcc warns about this. While this may be the intention +/// in some programming contexts, it may not have been what the programmer +/// really wanted in all contexts. So it's best to implement this as a tidy +/// check to start with, get some experience using it, then perhaps promote +/// this check to a diagnostic. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-complex-conditions.html +class ComplexConditionsCheck : public ClangTidyCheck { +public: + ComplexConditionsCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_COMPLEXCONDITIONSCHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/ComplexConditionsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ComplexConditionsCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/ComplexConditionsCheck.cpp @@ -0,0 +1,36 @@ +//===--- ComplexConditionsCheck.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 "ComplexConditionsCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +void ComplexConditionsCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(binaryOperator(hasAnyOperatorName("<", ">", "<=", ">="), + has(binaryOperator(hasAnyOperatorName( + "<", ">", "<=", ">=")))) + .bind("complex_binop"), + this); +} + +void ComplexConditionsCheck::check(const MatchFinder::MatchResult &Result) { + const auto *BinOp = Result.Nodes.getNodeAs("complex_binop"); + if (BinOp) + diag(BinOp->getBeginLoc(), + "comparisons like `X<=Y<=Z` have no mathematical meaning"); +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -69,6 +69,12 @@ The improvements are... +- New :doc:`misc-complex-conditions + ` check. + + Finds complex conditions using ``<``, ``>``, ``<=``, and ``>=`` that could be + interpreted ambiguously. + Improvements to include-fixer ----------------------------- diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-complex-conditions.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-complex-conditions.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-complex-conditions.rst @@ -0,0 +1,48 @@ +.. title:: clang-tidy - bugprone-complex-conditions + +bugprone-complex-conditions +=========================== + +Finds complex conditions using ``<``, ``>``, ``<=``, and ``>=`` that could be +interpreted ambiguously. + + +It's possible to write an expression as ``(x <= y <= z)``, which has no +meaning. This expression is parsed as ``((x <= y) x<= z)``. A compare returns +a boolean value when used in an integral context, is promoted to ``0`` +(for false) or ``1`` (for true). So when ``x <= y`` the expression becomes +``(1 <= z)`` and when ``(x > y)`` the expression becomes ``(0 <= z)``. + +While it's possible this is precisely what the programmer wanted, it's also +possible this was a programming mistake, so this is appropriately flagged as +a warning when explicitly enabled. + +Example cases warned include: + +.. code-block:: c + + int a = p1 < p2 < p3; + + int b = (p1 > p2) < p3 < p4; + + if (p1 > p2 < p3) { + ... + } + +Example cases not warned, since the ambigiuty has been removed, include: + +.. code-block:: c + + int a = p1 < (p2 < p3); + + int b = ((p1 > p2) < p3) < p4; + + int c = ((p1 > p2) < (p3 < p4); + + if (p1 > (p2 < p3)) { + ... + } + +This check does not include a fixit since it's not reasonable for the checker +to guess at the programmer's intention. + diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -51,6 +51,7 @@ `bugprone-bad-signal-to-kill-thread `_, `bugprone-bool-pointer-implicit-conversion `_, "Yes" `bugprone-branch-clone `_, + `bugprone-complex-conditions `_, `bugprone-copy-constructor-init `_, "Yes" `bugprone-dangling-handle `_, `bugprone-dynamic-static-initializers `_, @@ -70,7 +71,7 @@ `bugprone-misplaced-widening-cast `_, `bugprone-move-forwarding-reference `_, "Yes" `bugprone-multiple-statement-macro `_, - `bugprone-no-escape `_, "Yes" + `bugprone-no-escape `_, `bugprone-not-null-terminated-result `_, "Yes" `bugprone-parent-virtual-call `_, "Yes" `bugprone-posix-return `_, "Yes" diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-complex-conditions.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone-complex-conditions.c new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-complex-conditions.c @@ -0,0 +1,118 @@ +// RUN: %check_clang_tidy %s bugprone-complex-conditions %t + +int case1(int p1, int p2, int p3) { + if (p1 < p2 < p3) + return 1; + return 0; + // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [bugprone-complex-conditions] +} + +int case2(int p1, int p2, int p3) { + // no warning + if ((p1 < p2) && (p2 < p3)) + return 1; + return 0; +} + +int case3(int p1, int p2, int p3) { + // no warning + if ((p1 < p2) && (p2)) + return 1; + return 0; +} + +int case4(int p1, int p2, int p3) { + // no warning + if ((p1) && (p3 < p2)) + return 1; + return 0; +} + +int case5(int p1, int p2, int p3) { + while (p1 < p3 < p2) + return 1; + return 0; + // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [bugprone-complex-conditions] +} + +int case6(int p1, int p2, int p3) { + // should not warn + while (p1 && p3 < p2) + return 1; + return 0; +} + +int case7(int p1, int p2, int p3) { + // should not warn + while ((p1 < p3) < p2) + return 1; + return 0; +} + +int case8(int p1, int p2, int p3) { + int ret = p1 < p2 < p3; + // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [bugprone-complex-conditions] + return ret; +} + +int case9(int p1, int p2, int p3) { + int ret = (p1 < p2) < p3 < p1; + // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [bugprone-complex-conditions] + return ret; +} + +int case10(int p1, int p2, int p3) { + if (p1 <= p2 < p3) + return 1; + return 0; + // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [bugprone-complex-conditions] +} + +int case11(int p1, int p2, int p3) { + if (p1 < p2 <= p3) + return 1; + return 0; + // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [bugprone-complex-conditions] +} + +int case12(int p1, int p2, int p3) { + if (p1 <= p2 <= p3) + return 1; + return 0; + // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [bugprone-complex-conditions] +} + +int case13(int p1, int p2, int p3) { + if (p1 > p2 < p3) + return 1; + return 0; + // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [bugprone-complex-conditions] +} + +int case14(int p1, int p2, int p3) { + if (p1 > p2 > p3) + return 1; + return 0; + // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [bugprone-complex-conditions] +} + +int case15(int p1, int p2, int p3) { + if (p1 >= p2 > p3) + return 1; + return 0; + // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [bugprone-complex-conditions] +} + +int case16(int p1, int p2, int p3) { + if (p1 >= p2 >= p3) + return 1; + return 0; + // CHECK-MESSAGES: warning: comparisons like `X<=Y<=Z` have no mathematical meaning [bugprone-complex-conditions] +} + +int case17(int p1, int p2, int p3) { + // should not warn + if (p1 >= p2 || p3) + return 1; + return 0; +}