vabridgers updated this revision to Diff 281937.
vabridgers marked 6 inline comments as done.
vabridgers added a comment.
Address most review comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84898/new/
https://reviews.llvm.org/D84898
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/ComplexConditionsCheck.cpp
clang-tools-extra/clang-tidy/bugprone/ComplexConditionsCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone-complex-conditions.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone-complex-conditions.c
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-complex-conditions.c
===================================================================
--- /dev/null
+++ 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;
+}
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
@@ -51,6 +51,7 @@
`bugprone-bad-signal-to-kill-thread <bugprone-bad-signal-to-kill-thread.html>`_,
`bugprone-bool-pointer-implicit-conversion <bugprone-bool-pointer-implicit-conversion.html>`_, "Yes"
`bugprone-branch-clone <bugprone-branch-clone.html>`_,
+ `bugprone-complex-conditions <bugprone-complex-conditions.html>`_,
`bugprone-copy-constructor-init <bugprone-copy-constructor-init.html>`_, "Yes"
`bugprone-dangling-handle <bugprone-dangling-handle.html>`_,
`bugprone-dynamic-static-initializers <bugprone-dynamic-static-initializers.html>`_,
@@ -70,7 +71,7 @@
`bugprone-misplaced-widening-cast <bugprone-misplaced-widening-cast.html>`_,
`bugprone-move-forwarding-reference <bugprone-move-forwarding-reference.html>`_, "Yes"
`bugprone-multiple-statement-macro <bugprone-multiple-statement-macro.html>`_,
- `bugprone-no-escape <bugprone-no-escape.html>`_, "Yes"
+ `bugprone-no-escape <bugprone-no-escape.html>`_,
`bugprone-not-null-terminated-result <bugprone-not-null-terminated-result.html>`_, "Yes"
`bugprone-parent-virtual-call <bugprone-parent-virtual-call.html>`_, "Yes"
`bugprone-posix-return <bugprone-posix-return.html>`_, "Yes"
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-complex-conditions.rst
===================================================================
--- /dev/null
+++ 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.
+
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -69,6 +69,12 @@
The improvements are...
+- New :doc:`misc-complex-conditions
+ <clang-tidy/checks/misc-complex-conditions>` check.
+
+ Finds complex conditions using `<`, `>`, `<=`, and `>=` that could be
+ interpreted ambiguously.
+
Improvements to include-fixer
-----------------------------
Index: clang-tools-extra/clang-tidy/bugprone/ComplexConditionsCheck.h
===================================================================
--- /dev/null
+++ 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
Index: clang-tools-extra/clang-tidy/bugprone/ComplexConditionsCheck.cpp
===================================================================
--- /dev/null
+++ 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<Stmt>("complex_binop");
+ if (BinOp)
+ diag(BinOp->getBeginLoc(),
+ "comparisons like `X<=Y<=Z` have no mathematical meaning");
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ 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
Index: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ 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<BranchCloneCheck>(
"bugprone-branch-clone");
+ CheckFactories.registerCheck<ComplexConditionsCheck>(
+ "bugprone-complex-conditions");
CheckFactories.registerCheck<CopyConstructorInitCheck>(
"bugprone-copy-constructor-init");
CheckFactories.registerCheck<DanglingHandleCheck>(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
