vabridgers created this revision.
Herald added subscribers: cfe-commits, phosek, Charusso, mgorny.
Herald added a project: clang.
vabridgers requested review of this revision.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D84898
Files:
clang-tools-extra/clang-tidy/misc/CMakeLists.txt
clang-tools-extra/clang-tidy/misc/ComplexConditionsCheck.cpp
clang-tools-extra/clang-tidy/misc/ComplexConditionsCheck.h
clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/docs/clang-tidy/checks/misc-complex-conditions.rst
clang-tools-extra/test/clang-tidy/checkers/misc-complex-conditions.c
Index: clang-tools-extra/test/clang-tidy/checkers/misc-complex-conditions.c
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-complex-conditions.c
@@ -0,0 +1,118 @@
+// RUN: %check_clang_tidy %s misc-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 [misc-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 [misc-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 [misc-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 [misc-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 [misc-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 [misc-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 [misc-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 [misc-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 [misc-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 [misc-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 [misc-complex-conditions]
+}
+
+int case17(int p1, int p2, int p3) {
+ // should not warn
+ if (p1 >= p2 || p3)
+ return 1;
+ return 0;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/misc-complex-conditions.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/misc-complex-conditions.rst
@@ -0,0 +1,46 @@
+.. title:: clang-tidy - misc-complex-conditions
+
+misc-complex-conditions
+=======================
+
+Detects conditions that have no mathematical meaing.
+
+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.
+
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -194,6 +194,7 @@
`llvmlibc-callee-namespace <llvmlibc-callee-namespace.html>`_,
`llvmlibc-implementation-in-namespace <llvmlibc-implementation-in-namespace.html>`_,
`llvmlibc-restrict-system-libc-headers <llvmlibc-restrict-system-libc-headers.html>`_, "Yes"
+ `misc-complex-conditions <misc-complex-conditions.html>`_,
`misc-definitions-in-headers <misc-definitions-in-headers.html>`_, "Yes"
`misc-misplaced-const <misc-misplaced-const.html>`_,
`misc-new-delete-overloads <misc-new-delete-overloads.html>`_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -67,7 +67,14 @@
Improvements to clang-tidy
--------------------------
-The improvements are...
+- The 'fuchsia-restrict-system-headers' check was renamed to :doc:`portability-restrict-system-includes
+ <clang-tidy/checks/portability-restrict-system-includes>`
+
+- New :doc:`misc-complex-conditions
+ <clang-tidy/checks/misc-complex-conditions>` check.
+
+ Finds complex conditions using <, >, <=, and >= that have no mathematical
+ meaning.
Improvements to include-fixer
-----------------------------
Index: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
@@ -9,6 +9,7 @@
#include "../ClangTidy.h"
#include "../ClangTidyModule.h"
#include "../ClangTidyModuleRegistry.h"
+#include "ComplexConditionsCheck.h"
#include "DefinitionsInHeadersCheck.h"
#include "MisplacedConstCheck.h"
#include "NewDeleteOverloadsCheck.h"
@@ -31,6 +32,8 @@
class MiscModule : public ClangTidyModule {
public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+ CheckFactories.registerCheck<ComplexConditionsCheck>(
+ "misc-complex-conditions");
CheckFactories.registerCheck<DefinitionsInHeadersCheck>(
"misc-definitions-in-headers");
CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
Index: clang-tools-extra/clang-tidy/misc/ComplexConditionsCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/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_MISC_COMPLEXCONDITIONSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COMPLEXCONDITIONSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// 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/misc-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 misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_COMPLEXCONDITIONSCHECK_H
Index: clang-tools-extra/clang-tidy/misc/ComplexConditionsCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/misc/ComplexConditionsCheck.cpp
@@ -0,0 +1,42 @@
+//===--- 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 misc {
+
+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 *If = Result.Nodes.getNodeAs<IfStmt>("complex_binop");
+ const auto *Statement = Result.Nodes.getNodeAs<Stmt>("complex_binop");
+ if (If) {
+ diag(If->getBeginLoc(),
+ "comparisons like `X<=Y<=Z` have no mathematical meaning");
+ }
+ if (Statement) {
+ diag(Statement->getBeginLoc(),
+ "comparisons like `X<=Y<=Z` have no mathematical meaning");
+ }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/misc/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/misc/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/misc/CMakeLists.txt
@@ -4,6 +4,7 @@
)
add_clang_library(clangTidyMiscModule
+ ComplexConditionsCheck.cpp
DefinitionsInHeadersCheck.cpp
MiscTidyModule.cpp
MisplacedConstCheck.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
