PiotrZSL created this revision.
PiotrZSL added reviewers: njames93, carlosgalvezp.
Herald added a subscriber: xazax.hun.
Herald added a project: All.
PiotrZSL requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Detects when a variable is both incremented/decremented and referenced inside a
complex condition and suggests moving them outside to avoid ambiguity in the
variable's value.
Depends on D148995 <https://reviews.llvm.org/D148995>
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D149015
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.cpp
clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.h
clang-tools-extra/clang-tidy/utils/CMakeLists.txt
clang-tools-extra/clang-tidy/utils/Matchers.cpp
clang-tools-extra/clang-tidy/utils/Matchers.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/inc-dec-in-conditions.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/inc-dec-in-conditions.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/inc-dec-in-conditions.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/inc-dec-in-conditions.cpp
@@ -0,0 +1,70 @@
+// RUN: %check_clang_tidy %s bugprone-inc-dec-in-conditions %t
+
+template<typename T>
+struct Iterator {
+ Iterator operator++(int);
+ Iterator operator--(int);
+ Iterator& operator++();
+ Iterator& operator--();
+ T operator*();
+ bool operator==(Iterator) const;
+ bool operator!=(Iterator) const;
+};
+
+template<typename T>
+struct Container {
+ Iterator<T> begin();
+ Iterator<T> end();
+};
+
+bool f(int x) {
+ return (++x != 5 or x == 10);
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: incrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool f2(int x) {
+ return (x++ != 5 or x == 10);
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: incrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool c(Container<int> x) {
+ auto it = x.begin();
+ return (it++ != x.end() and *it);
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: incrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool c2(Container<int> x) {
+ auto it = x.begin();
+ return (++it != x.end() and *it);
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: incrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool d(int x) {
+ return (--x != 5 or x == 10);
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: decrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool d2(int x) {
+ return (x-- != 5 or x == 10);
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: decrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool g(Container<int> x) {
+ auto it = x.begin();
+ return (it-- != x.end() and *it);
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: decrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool g2(Container<int> x) {
+ auto it = x.begin();
+ return (--it != x.end() and *it);
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: decrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
+
+bool doubleCheck(Container<int> x) {
+ auto it = x.begin();
+ auto it2 = x.begin();
+ return (--it != x.end() and ++it2 != x.end()) and (*it == *it2);
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: decrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+ // CHECK-MESSAGES: :[[@LINE-2]]:31: warning: incrementing and referencing a variable in a complex condition can cause unintended side-effects due to C++'s order of evaluation, consider moving the modification outside of the condition to avoid misunderstandings [bugprone-inc-dec-in-conditions]
+}
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
@@ -92,6 +92,7 @@
`bugprone-forwarding-reference-overload <bugprone/forwarding-reference-overload.html>`_,
`bugprone-implicit-widening-of-multiplication-result <bugprone/implicit-widening-of-multiplication-result.html>`_, "Yes"
`bugprone-inaccurate-erase <bugprone/inaccurate-erase.html>`_, "Yes"
+ `bugprone-inc-dec-in-conditions <bugprone/inc-dec-in-conditions.html>`_,
`bugprone-incorrect-roundings <bugprone/incorrect-roundings.html>`_,
`bugprone-infinite-loop <bugprone/infinite-loop.html>`_,
`bugprone-integer-division <bugprone/integer-division.html>`_,
@@ -478,7 +479,7 @@
`cppcoreguidelines-explicit-virtual-functions <cppcoreguidelines/explicit-virtual-functions.html>`_, `modernize-use-override <modernize/use-override.html>`_, "Yes"
`cppcoreguidelines-macro-to-enum <cppcoreguidelines/macro-to-enum.html>`_, `modernize-macro-to-enum <modernize/macro-to-enum.html>`_, "Yes"
`cppcoreguidelines-non-private-member-variables-in-classes <cppcoreguidelines/non-private-member-variables-in-classes.html>`_, `misc-non-private-member-variables-in-classes <misc/non-private-member-variables-in-classes.html>`_,
- `cppcoreguidelines-use-default-member-init <cppcoreguidelines/use-default-member-init.html>`_, `modernize-use-default-member-init <modernize/use-default-member-init.html>`_,
+ `cppcoreguidelines-use-default-member-init <cppcoreguidelines/use-default-member-init.html>`_, `modernize-use-default-member-init <modernize/use-default-member-init.html>`_, "Yes"
`fuchsia-header-anon-namespaces <fuchsia/header-anon-namespaces.html>`_, `google-build-namespaces <google/build-namespaces.html>`_,
`google-readability-braces-around-statements <google/readability-braces-around-statements.html>`_, `readability-braces-around-statements <readability/braces-around-statements.html>`_, "Yes"
`google-readability-function-size <google/readability-function-size.html>`_, `readability-function-size <readability/function-size.html>`_,
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/inc-dec-in-conditions.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/inc-dec-in-conditions.rst
@@ -0,0 +1,67 @@
+.. title:: clang-tidy - bugprone-inc-dec-in-conditions
+
+bugprone-inc-dec-in-conditions
+==============================
+
+Detects when a variable is both incremented/decremented and referenced inside a
+complex condition and suggests moving them outside to avoid ambiguity in the
+variable's value.
+
+When a variable is modified and also used in a complex condition, it can lead to
+unexpected behavior. The side-effect of changing the variable's value within the
+condition can make the code difficult to reason about. Additionally, the
+developer's intended timing for the modification of the variable may not be
+clear, leading to misunderstandings and errors. This can be particularly
+problematic when the condition involves logical operators like ``&&`` and
+``||``, where the order of evaluation can further complicate the situation.
+
+Consider the following example:
+
+.. code-block:: c++
+
+ int i = 0;
+ // ...
+ if (i++ < 5 && i > 0) {
+ // do something
+ }
+
+In this example, the result of the expression may not be what the developer
+intended. The original intention of the developer could be to increment ``i``
+after the entire condition is evaluated, but in reality, i will be incremented
+before ``i > 0`` is executed. This can lead to unexpected behavior and bugs in
+the code. To fix this issue, the developer should separate the increment
+operation from the condition and perform it separately. For example, they can
+increment ``i`` in a separate statement before or after the condition is
+evaluated. This ensures that the value of ``i`` is predictable and consistent
+throughout the code.
+
+.. code-block:: c++
+
+ int i = 0;
+ // ...
+ i++;
+ if (i <= 5 && i > 0) {
+ // do something
+ }
+
+Another common issue occurs when multiple increments or decrements are performed
+on the same variable inside a complex condition. For example:
+
+.. code-block:: c++
+
+ int i = 5;
+ // ...
+ if (i++ < 5 || --i > 2) {
+ // do something
+ }
+
+there is a potential issue with this code due to the order of evaluation in C++.
+The ``||`` operator used in the condition statement guarantees that if the first
+operand evaluates to ``true``, the second operand will not be evaluated. This
+means that if ``i`` were initially ``5``, the first operand ``i < 5`` would
+evaluate to ``false`` and the second operand ``i > 2`` would not be evaluated.
+As a result, the decrement operation ``--i`` would not be executed and ``i``
+would hold value ``6``, which may not be the intended behavior for the developer.
+
+To avoid this potential issue, the both increment and decrement operation on
+``i`` should be moved outside the condition statement.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -106,6 +106,13 @@
New checks
^^^^^^^^^^
+- New :doc:`bugprone-inc-dec-in-conditions
+ <clang-tidy/checks/bugprone/inc-dec-in-conditions>` check.
+
+ Detects when a variable is both incremented/decremented and referenced inside
+ a complex condition and suggests moving them outside to avoid ambiguity in
+ the variable's value.
+
- New :doc:`bugprone-non-zero-enum-to-bool-conversion
<clang-tidy/checks/bugprone/non-zero-enum-to-bool-conversion>` check.
Index: clang-tools-extra/clang-tidy/utils/Matchers.h
===================================================================
--- clang-tools-extra/clang-tidy/utils/Matchers.h
+++ clang-tools-extra/clang-tidy/utils/Matchers.h
@@ -138,6 +138,23 @@
new MatchesAnyListedNameMatcher(NameList));
}
+struct NotIdenticalStatementsPredicate {
+ bool
+ operator()(const clang::ast_matchers::internal::BoundNodesMap &Nodes) const;
+
+ std::string ID;
+ ::clang::DynTypedNode Node;
+ ASTContext *Context;
+};
+
+AST_MATCHER_P(Stmt, isStatementIdenticalToBoundNode, std::string, ID) {
+ NotIdenticalStatementsPredicate Predicate;
+ Predicate.ID = ID;
+ Predicate.Node = ::clang::DynTypedNode::create(Node);
+ Predicate.Context = &(Finder->getASTContext());
+ return Builder->removeBindings(Predicate);
+}
+
} // namespace clang::tidy::matchers
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_UTILS_MATCHERS_H
Index: clang-tools-extra/clang-tidy/utils/Matchers.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/utils/Matchers.cpp
@@ -0,0 +1,20 @@
+//===---------- Matchers.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 "Matchers.h"
+#include "ASTUtils.h"
+
+namespace clang::tidy::matchers {
+
+bool NotIdenticalStatementsPredicate::operator()(
+ const clang::ast_matchers::internal::BoundNodesMap &Nodes) const {
+ return !utils::areStatementsIdentical(
+ Node.get<Stmt>(), Nodes.getNode(ID).get<Stmt>(), *Context);
+}
+
+} // namespace clang::tidy::matchers
Index: clang-tools-extra/clang-tidy/utils/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/utils/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/utils/CMakeLists.txt
@@ -16,6 +16,7 @@
IncludeInserter.cpp
IncludeSorter.cpp
LexerUtils.cpp
+ Matchers.cpp
NamespaceAliaser.cpp
OptionsUtils.cpp
RenamerClangTidyCheck.cpp
Index: clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.h
@@ -0,0 +1,35 @@
+//===--- IncDecInConditionsCheck.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_INCDECINCONDITIONSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCDECINCONDITIONSCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Detects when a variable is both incremented/decremented and referenced
+/// inside a complex condition and suggests moving them outside to avoid
+/// ambiguity in the variable's value.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/inc-dec-in-conditions.html
+class IncDecInConditionsCheck : public ClangTidyCheck {
+public:
+ IncDecInConditionsCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ std::optional<TraversalKind> getCheckTraversalKind() const override {
+ return TK_IgnoreUnlessSpelledInSource;
+ }
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INCDECINCONDITIONSCHECK_H
Index: clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/IncDecInConditionsCheck.cpp
@@ -0,0 +1,82 @@
+//===--- IncDecInConditionsCheck.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 "IncDecInConditionsCheck.h"
+#include "../utils/Matchers.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+AST_MATCHER(BinaryOperator, isLogicalOperator) { return Node.isLogicalOp(); }
+
+AST_MATCHER(UnaryOperator, isUnaryPrePostOperator) {
+ return Node.isPrefix() || Node.isPostfix();
+}
+
+AST_MATCHER(CXXOperatorCallExpr, isPrePostOperator) {
+ return Node.getOperator() == OO_PlusPlus ||
+ Node.getOperator() == OO_MinusMinus;
+}
+
+void IncDecInConditionsCheck::registerMatchers(MatchFinder *Finder) {
+ auto OperatorMacher = expr(
+ anyOf(binaryOperator(anyOf(isComparisonOperator(), isLogicalOperator())),
+ cxxOperatorCallExpr(isComparisonOperator())));
+
+ Finder->addMatcher(
+ expr(
+ OperatorMacher, unless(isExpansionInSystemHeader()),
+ unless(hasAncestor(OperatorMacher)), expr().bind("parent"),
+
+ forEachDescendant(
+ expr(anyOf(unaryOperator(isUnaryPrePostOperator(),
+ hasUnaryOperand(expr().bind("operand"))),
+ cxxOperatorCallExpr(
+ isPrePostOperator(),
+ hasUnaryOperand(expr().bind("operand")))),
+ hasAncestor(
+ expr(equalsBoundNode("parent"),
+ hasDescendant(
+ expr(unless(equalsBoundNode("operand")),
+ matchers::isStatementIdenticalToBoundNode(
+ "operand"))
+ .bind("second")))))
+ .bind("operator"))),
+ this);
+}
+
+void IncDecInConditionsCheck::check(const MatchFinder::MatchResult &Result) {
+
+ SourceLocation ExprLoc;
+ bool IsIncrementOp = false;
+
+ if (const auto *MatchedDecl =
+ Result.Nodes.getNodeAs<CXXOperatorCallExpr>("operator")) {
+ ExprLoc = MatchedDecl->getExprLoc();
+ IsIncrementOp = (MatchedDecl->getOperator() == OO_PlusPlus);
+ } else if (const auto *MatchedDecl =
+ Result.Nodes.getNodeAs<UnaryOperator>("operator")) {
+ ExprLoc = MatchedDecl->getExprLoc();
+ IsIncrementOp = MatchedDecl->isIncrementOp();
+ } else
+ return;
+
+ diag(ExprLoc,
+ "%select{decrementing|incrementing}0 and referencing a variable in a "
+ "complex condition can cause unintended side-effects due to C++'s order "
+ "of evaluation, consider moving the modification outside of the "
+ "condition to avoid misunderstandings")
+ << IsIncrementOp;
+ diag(Result.Nodes.getNodeAs<Expr>("second")->getExprLoc(),
+ "variable is referenced here", DiagnosticIDs::Note);
+}
+
+} // namespace clang::tidy::bugprone
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
@@ -21,6 +21,7 @@
ForwardingReferenceOverloadCheck.cpp
ImplicitWideningOfMultiplicationResultCheck.cpp
InaccurateEraseCheck.cpp
+ IncDecInConditionsCheck.cpp
IncorrectRoundingsCheck.cpp
InfiniteLoopCheck.cpp
IntegerDivisionCheck.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
@@ -26,6 +26,7 @@
#include "ForwardingReferenceOverloadCheck.h"
#include "ImplicitWideningOfMultiplicationResultCheck.h"
#include "InaccurateEraseCheck.h"
+#include "IncDecInConditionsCheck.h"
#include "IncorrectRoundingsCheck.h"
#include "InfiniteLoopCheck.h"
#include "IntegerDivisionCheck.h"
@@ -114,6 +115,8 @@
"bugprone-implicit-widening-of-multiplication-result");
CheckFactories.registerCheck<InaccurateEraseCheck>(
"bugprone-inaccurate-erase");
+ CheckFactories.registerCheck<IncDecInConditionsCheck>(
+ "bugprone-inc-dec-in-conditions");
CheckFactories.registerCheck<IncorrectRoundingsCheck>(
"bugprone-incorrect-roundings");
CheckFactories.registerCheck<InfiniteLoopCheck>("bugprone-infinite-loop");
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
