[clang-tidy] Add misc-bool-bitwise-operation check #167552
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
✅ With the latest revision this PR passed the C/C++ code linter. |
Member
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Denis Mikhailov (denzor200) ChangesPrevious PR had some implementations problems, so I recreated this one. Two major updates: -Implemented diagnostics for structs members I've tested it on llvm codebase with It provided ~20813 warnings and ~653 fixits. Fixes: #40307 Patch is 105.73 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/167552.diff 17 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.cpp b/clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.cpp
new file mode 100644
index 0000000000000..1507d51320c7c
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.cpp
@@ -0,0 +1,307 @@
+//===--- BoolBitwiseOperationCheck.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 "BoolBitwiseOperationCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+#include <array>
+#include <optional>
+#include <utility>
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::misc {
+
+static const DynTypedNode *ignoreParensTowardsTheRoot(const DynTypedNode *N,
+ ASTContext *AC) {
+ if (const auto *S = N->get<Stmt>()) {
+ if (isa<ParenExpr>(S)) {
+ auto Parents = AC->getParents(*S);
+ for (const auto &Parent : Parents) {
+ return ignoreParensTowardsTheRoot(&Parent, AC);
+ }
+ }
+ }
+ return N;
+}
+
+static bool assignsToBoolean(const BinaryOperator *BinOp, ASTContext *AC) {
+ TraversalKindScope RAII(*AC, TK_AsIs);
+ auto Parents = AC->getParents(*BinOp);
+
+ for (const auto &Parent : Parents) {
+ if (const auto *S = ignoreParensTowardsTheRoot(&Parent, AC)->get<Stmt>()) {
+ if (const auto *ICE = dyn_cast<ImplicitCastExpr>(S)) {
+ if (ICE->getType().getDesugaredType(*AC)->isBooleanType())
+ return true;
+ }
+ }
+ }
+
+ return false;
+}
+
+constexpr std::array<std::pair<llvm::StringRef, llvm::StringRef>, 8U>
+ OperatorsTransformation{{{"|", "||"},
+ {"|=", "||"},
+ {"&", "&&"},
+ {"&=", "&&"},
+ {"bitand", "and"},
+ {"and_eq", "and"},
+ {"bitor", "or"},
+ {"or_eq", "or"}}};
+
+static llvm::StringRef translate(llvm::StringRef Value) {
+ for (const auto &[Bitwise, Logical] : OperatorsTransformation) {
+ if (Value == Bitwise)
+ return Logical;
+ }
+
+ return {};
+}
+
+static bool isBooleanBitwise(const BinaryOperator *BinOp, ASTContext *AC,
+ std::optional<bool> &RootAssignsToBoolean) {
+ if (!BinOp)
+ return false;
+
+ for (const auto &[Bitwise, _] : OperatorsTransformation) {
+ if (BinOp->getOpcodeStr() == Bitwise) {
+ bool IsBooleanLHS = BinOp->getLHS()
+ ->IgnoreImpCasts()
+ ->getType()
+ .getDesugaredType(*AC)
+ ->isBooleanType();
+ bool IsBooleanRHS = BinOp->getRHS()
+ ->IgnoreImpCasts()
+ ->getType()
+ .getDesugaredType(*AC)
+ ->isBooleanType();
+ if (IsBooleanLHS && IsBooleanRHS) {
+ RootAssignsToBoolean = RootAssignsToBoolean.value_or(false);
+ return true;
+ }
+ if (((IsBooleanLHS || IsBooleanRHS) && assignsToBoolean(BinOp, AC)) ||
+ RootAssignsToBoolean.value_or(false)) {
+ RootAssignsToBoolean = RootAssignsToBoolean.value_or(true);
+ return true;
+ }
+ if (BinOp->isCompoundAssignmentOp() && IsBooleanLHS) {
+ RootAssignsToBoolean = RootAssignsToBoolean.value_or(true);
+ return true;
+ }
+
+ std::optional<bool> DummyFlag = false;
+ IsBooleanLHS =
+ IsBooleanLHS ||
+ isBooleanBitwise(
+ dyn_cast<BinaryOperator>(BinOp->getLHS()->IgnoreParenImpCasts()),
+ AC, DummyFlag);
+ IsBooleanRHS =
+ IsBooleanRHS ||
+ isBooleanBitwise(
+ dyn_cast<BinaryOperator>(BinOp->getRHS()->IgnoreParenImpCasts()),
+ AC, DummyFlag);
+
+ if (IsBooleanLHS && IsBooleanRHS) {
+ RootAssignsToBoolean = RootAssignsToBoolean.value_or(false);
+ return true;
+ }
+ }
+ }
+ return false;
+}
+
+static const Expr *getAcceptableCompoundsLHS(const BinaryOperator *BinOp) {
+ assert(BinOp->isCompoundAssignmentOp());
+
+ if (const auto *DeclRefLHS =
+ dyn_cast<DeclRefExpr>(BinOp->getLHS()->IgnoreImpCasts()))
+ return DeclRefLHS;
+ if (const auto *MemberLHS =
+ dyn_cast<MemberExpr>(BinOp->getLHS()->IgnoreImpCasts()))
+ return MemberLHS;
+
+ return nullptr;
+}
+
+BoolBitwiseOperationCheck::BoolBitwiseOperationCheck(StringRef Name,
+ ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ UnsafeMode(Options.get("UnsafeMode", false)),
+ IgnoreMacros(Options.get("IgnoreMacros", false)) {}
+
+void BoolBitwiseOperationCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "UnsafeMode", UnsafeMode);
+ Options.store(Opts, "IgnoreMacros", IgnoreMacros);
+}
+
+void BoolBitwiseOperationCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ binaryOperator(unless(isExpansionInSystemHeader()),
+ unless(hasParent(binaryOperator())) // ignoring parenExpr
+ )
+ .bind("binOpRoot"),
+ this);
+}
+
+void BoolBitwiseOperationCheck::emitWarningAndChangeOperatorsIfPossible(
+ const BinaryOperator *BinOp, const BinaryOperator *ParentBinOp,
+ const clang::SourceManager &SM, clang::ASTContext &Ctx) {
+ auto DiagEmitter = [BinOp, this] {
+ return diag(BinOp->getOperatorLoc(),
+ "use logical operator '%0' for boolean semantics instead of "
+ "bitwise operator '%1'")
+ << translate(BinOp->getOpcodeStr()) << BinOp->getOpcodeStr();
+ };
+
+ const bool HasVolatileOperand = llvm::any_of(
+ std::array{BinOp->getLHS(), BinOp->getRHS()}, [&](const Expr *E) {
+ return E->IgnoreImpCasts()
+ ->getType()
+ .getDesugaredType(Ctx)
+ .isVolatileQualified();
+ });
+ if (HasVolatileOperand) {
+ DiagEmitter();
+ return;
+ }
+
+ const bool HasSideEffects = BinOp->getRHS()->HasSideEffects(
+ Ctx, /*IncludePossibleEffects=*/!UnsafeMode);
+ if (HasSideEffects) {
+ DiagEmitter();
+ return;
+ }
+
+ SourceLocation Loc = BinOp->getOperatorLoc();
+
+ if (Loc.isInvalid() || Loc.isMacroID()) {
+ IgnoreMacros || DiagEmitter();
+ return;
+ }
+
+ Loc = SM.getSpellingLoc(Loc);
+ if (Loc.isInvalid() || Loc.isMacroID()) {
+ IgnoreMacros || DiagEmitter();
+ return;
+ }
+
+ const CharSourceRange TokenRange = CharSourceRange::getTokenRange(Loc);
+ if (TokenRange.isInvalid()) {
+ IgnoreMacros || DiagEmitter();
+ return;
+ }
+
+ const StringRef FixSpelling =
+ translate(Lexer::getSourceText(TokenRange, SM, Ctx.getLangOpts()));
+
+ if (FixSpelling.empty()) {
+ DiagEmitter();
+ return;
+ }
+
+ FixItHint InsertEqual;
+ if (BinOp->isCompoundAssignmentOp()) {
+ const auto *LHS = getAcceptableCompoundsLHS(BinOp);
+ if (!LHS) {
+ DiagEmitter();
+ return;
+ }
+ const SourceLocation LocLHS = LHS->getEndLoc();
+ if (LocLHS.isInvalid() || LocLHS.isMacroID()) {
+ IgnoreMacros || DiagEmitter();
+ return;
+ }
+ const SourceLocation InsertLoc =
+ clang::Lexer::getLocForEndOfToken(LocLHS, 0, SM, Ctx.getLangOpts());
+ if (InsertLoc.isInvalid() || InsertLoc.isMacroID()) {
+ IgnoreMacros || DiagEmitter();
+ return;
+ }
+ auto SourceText = static_cast<std::string>(Lexer::getSourceText(
+ CharSourceRange::getTokenRange(LHS->getSourceRange()), SM,
+ Ctx.getLangOpts()));
+ llvm::erase_if(SourceText,
+ [](unsigned char Ch) { return std::isspace(Ch); });
+ InsertEqual = FixItHint::CreateInsertion(InsertLoc, " = " + SourceText);
+ }
+
+ auto ReplaceOperator = FixItHint::CreateReplacement(TokenRange, FixSpelling);
+
+ std::optional<BinaryOperatorKind> ParentOpcode;
+ if (ParentBinOp)
+ ParentOpcode = ParentBinOp->getOpcode();
+
+ const auto *RHS = dyn_cast<BinaryOperator>(BinOp->getRHS()->IgnoreImpCasts());
+ std::optional<BinaryOperatorKind> RHSOpcode;
+ if (RHS)
+ RHSOpcode = RHS->getOpcode();
+
+ const Expr *SurroundedExpr = nullptr;
+ if ((BinOp->getOpcode() == BO_Or && ParentOpcode == BO_LAnd) ||
+ (BinOp->getOpcode() == BO_And &&
+ llvm::is_contained({BO_Xor, BO_Or}, ParentOpcode))) {
+ const Expr *Side = ParentBinOp->getLHS()->IgnoreParenImpCasts() == BinOp
+ ? ParentBinOp->getLHS()
+ : ParentBinOp->getRHS();
+ SurroundedExpr = Side->IgnoreImpCasts();
+ assert(SurroundedExpr->IgnoreParens() == BinOp);
+ } else if (BinOp->getOpcode() == BO_AndAssign && RHSOpcode == BO_LOr)
+ SurroundedExpr = RHS;
+
+ if (isa_and_nonnull<ParenExpr>(SurroundedExpr))
+ SurroundedExpr = nullptr;
+
+ FixItHint InsertBrace1;
+ FixItHint InsertBrace2;
+ if (SurroundedExpr) {
+ const SourceLocation InsertFirstLoc = SurroundedExpr->getBeginLoc();
+ const SourceLocation InsertSecondLoc = clang::Lexer::getLocForEndOfToken(
+ SurroundedExpr->getEndLoc(), 0, SM, Ctx.getLangOpts());
+ if (InsertFirstLoc.isInvalid() || InsertFirstLoc.isMacroID() ||
+ InsertSecondLoc.isInvalid() || InsertSecondLoc.isMacroID()) {
+ IgnoreMacros || DiagEmitter();
+ return;
+ }
+ InsertBrace1 = FixItHint::CreateInsertion(InsertFirstLoc, "(");
+ InsertBrace2 = FixItHint::CreateInsertion(InsertSecondLoc, ")");
+ }
+
+ DiagEmitter() << InsertEqual << ReplaceOperator << InsertBrace1
+ << InsertBrace2;
+}
+
+void BoolBitwiseOperationCheck::visitBinaryTreesNode(
+ const BinaryOperator *BinOp, const BinaryOperator *ParentBinOp,
+ const clang::SourceManager &SM, clang::ASTContext &Ctx,
+ std::optional<bool> &RootAssignsToBoolean) {
+ if (!BinOp)
+ return;
+
+ if (isBooleanBitwise(BinOp, &Ctx, RootAssignsToBoolean))
+ emitWarningAndChangeOperatorsIfPossible(BinOp, ParentBinOp, SM, Ctx);
+
+ visitBinaryTreesNode(
+ dyn_cast<BinaryOperator>(BinOp->getLHS()->IgnoreParenImpCasts()), BinOp,
+ SM, Ctx, RootAssignsToBoolean);
+ visitBinaryTreesNode(
+ dyn_cast<BinaryOperator>(BinOp->getRHS()->IgnoreParenImpCasts()), BinOp,
+ SM, Ctx, RootAssignsToBoolean);
+}
+
+void BoolBitwiseOperationCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *BinOpRoot = Result.Nodes.getNodeAs<BinaryOperator>("binOpRoot");
+ const SourceManager &SM = *Result.SourceManager;
+ ASTContext &Ctx = *Result.Context;
+ std::optional<bool> RootAssignsToBoolean = std::nullopt;
+ visitBinaryTreesNode(BinOpRoot, nullptr, SM, Ctx, RootAssignsToBoolean);
+}
+
+} // namespace clang::tidy::misc
\ No newline at end of file
diff --git a/clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.h b/clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.h
new file mode 100644
index 0000000000000..082e4b48c27e8
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.h
@@ -0,0 +1,51 @@
+//===--- BoolBitwiseOperationCheck.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_BOOLBITWISEOPERATIONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_BOOLBITWISEOPERATIONCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::misc {
+
+/// Finds potentially inefficient use of bitwise operators such as ``&``, ``|``
+/// and their compound analogues on Boolean values where logical operators like
+/// ``&&`` and ``||`` would be more appropriate.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc/bool-bitwise-operation.html
+class BoolBitwiseOperationCheck : public ClangTidyCheck {
+public:
+ BoolBitwiseOperationCheck(StringRef Name, ClangTidyContext *Context);
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+ 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 || LangOpts.C99;
+ }
+ std::optional<TraversalKind> getCheckTraversalKind() const override {
+ return TK_IgnoreUnlessSpelledInSource;
+ }
+
+private:
+ void emitWarningAndChangeOperatorsIfPossible(
+ const BinaryOperator *BinOp, const BinaryOperator *ParentBinOp,
+ const clang::SourceManager &SM, clang::ASTContext &Ctx);
+ void visitBinaryTreesNode(const BinaryOperator *BinOp,
+ const BinaryOperator *ParentBinOp,
+ const clang::SourceManager &SM,
+ clang::ASTContext &Ctx,
+ std::optional<bool> &RootAssignsToBoolean);
+
+ bool UnsafeMode;
+ bool IgnoreMacros;
+};
+
+} // namespace clang::tidy::misc
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_BOOLBITWISEOPERATIONCHECK_H
\ No newline at end of file
diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
index 6214ee92927f2..7c494519d84f0 100644
--- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt
@@ -18,6 +18,7 @@ add_custom_target(genconfusable DEPENDS Confusables.inc)
set_target_properties(genconfusable PROPERTIES FOLDER "Clang Tools Extra/Sourcegenning")
add_clang_library(clangTidyMiscModule STATIC
+ BoolBitwiseOperationCheck.cpp
ConstCorrectnessCheck.cpp
CoroutineHostileRAIICheck.cpp
DefinitionsInHeadersCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
index 347fa2a82e43c..9312bdef50b9a 100644
--- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
@@ -9,6 +9,7 @@
#include "../ClangTidy.h"
#include "../ClangTidyModule.h"
#include "../ClangTidyModuleRegistry.h"
+#include "BoolBitwiseOperationCheck.h"
#include "ConfusableIdentifierCheck.h"
#include "ConstCorrectnessCheck.h"
#include "CoroutineHostileRAIICheck.h"
@@ -40,6 +41,8 @@ namespace misc {
class MiscModule : public ClangTidyModule {
public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+ CheckFactories.registerCheck<BoolBitwiseOperationCheck>(
+ "misc-bool-bitwise-operation");
CheckFactories.registerCheck<ConfusableIdentifierCheck>(
"misc-confusable-identifiers");
CheckFactories.registerCheck<ConstCorrectnessCheck>(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 666865cfb2fcd..4f83047e94da7 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -220,6 +220,13 @@ New checks
Finds calls to STL library iterator algorithms that could be replaced with
LLVM range-based algorithms from ``llvm/ADT/STLExtras.h``.
+- New :doc:`misc-bool-bitwise-operation
+ <clang-tidy/checks/misc/bool-bitwise-operation>` check.
+
+ Finds potentially inefficient use of bitwise operators such as ``&``, ``|``
+ and their compound analogues on Boolean values where logical operators like
+ ``&&`` and ``||`` would be more appropriate.
+
- New :doc:`misc-override-with-different-visibility
<clang-tidy/checks/misc/override-with-different-visibility>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index e2875604af72b..3e14aee287d64 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -262,6 +262,7 @@ Clang-Tidy Checks
:doc:`llvmlibc-implementation-in-namespace <llvmlibc/implementation-in-namespace>`,
:doc:`llvmlibc-inline-function-decl <llvmlibc/inline-function-decl>`, "Yes"
:doc:`llvmlibc-restrict-system-libc-headers <llvmlibc/restrict-system-libc-headers>`, "Yes"
+ :doc:`misc-bool-bitwise-operation <misc/bool-bitwise-operation>`, "Yes"
:doc:`misc-confusable-identifiers <misc/confusable-identifiers>`,
:doc:`misc-const-correctness <misc/const-correctness>`, "Yes"
:doc:`misc-coroutine-hostile-raii <misc/coroutine-hostile-raii>`,
@@ -608,4 +609,4 @@ Check aliases
:doc:`hicpp-use-override <hicpp/use-override>`, :doc:`modernize-use-override <modernize/use-override>`, "Yes"
:doc:`hicpp-vararg <hicpp/vararg>`, :doc:`cppcoreguidelines-pro-type-vararg <cppcoreguidelines/pro-type-vararg>`,
:doc:`llvm-else-after-return <llvm/else-after-return>`, :doc:`readability-else-after-return <readability/else-after-return>`, "Yes"
- :doc:`llvm-qualified-auto <llvm/qualified-auto>`, :doc:`readability-qualified-auto <readability/qualified-auto>`, "Yes"
+ :doc:`llvm-qualified-auto <llvm/qualified-auto>`, :doc:`readability-qualified-auto <readability/qualified-auto>`, "Yes"
\ No newline at end of file
diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/bool-bitwise-operation.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/bool-bitwise-operation.rst
new file mode 100644
index 0000000000000..480f92452f9c4
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc/bool-bitwise-operation.rst
@@ -0,0 +1,77 @@
+.. title:: clang-tidy - misc-bool-bitwise-operation
+
+misc-bool-bitwise-operation
+===========================
+
+Finds potentially inefficient use of bitwise operators such as ``&``, ``|``
+and their compound analogues on Boolean values where logical operators like
+``&&`` and ``||`` would be more appropriate.
+
+Bitwise operations on Booleans can incur unnecessary performance overhead due
+to implicit integer conversions and missed short-circuit evaluation.
+
+.. code-block:: c++
+
+ bool invalid = false;
+ invalid |= x > limit.x; // warning: use logical operator '||' for boolean semantics instead of bitwise operator '|='
+ // 400 | invalid |= x > limit.x;
+ // | ^~
+ // | = invalid ||
+ invalid |= y > limit.y; // warning: use logical operator '||' for boolean semantics instead of bitwise operator '|='
+ // 401 | invalid |= y > limit.y;
+ // | ^~
+ // | = invalid ||
+ invalid |= z > limit.z; // warning: use logical operator '||' for boolean semantics instead of bitwise operator '|='
+ // 402 | invalid |= z > limit.z;
+ // | ^~
+ // | = invalid ||
+ if (invalid) {
+ // error handling
+ }
+
+These 3 warnings suggest assigning the result of a logical ``||`` operation
+instead of using the ``|=`` operator:
+
+.. code-block:: c++
+
+ bool invalid = false;
+ invalid = invalid || x > limit.x;
+ invalid = invalid || y > limit.x;
+ invalid = invalid || z > limit.z;
+ if (invalid) {
+ // error handling
+ }
+
+It is not always a safe transformation though. The following case will warn
+without fix-it to preserve the semantics.
+
+.. code-block:: c++
+
+ volatile bool invalid = false;
+ invalid |= x > limit.x; // warning: use logical operator '||' for boolean semantics instead of bitwise operator '|='
+
+Limitations
+-----------
+
+* Bitwise operators inside templates aren't guaranteed to match.
+
+.. code-block:: c++
+
+ template <typename X>
+ void f(X a, X b) {
+ a | b; // the warning may not be emited
+ }
+
+Options
+-------
+
+.. option:: UnsafeMode
+
+ Enabling this option promotes more fix-it hints even when they might
+ change evaluation order or skip side effects. Default value is ...
[truncated]
|
vbvictor
requested review from
HerrCai0907,
PiotrZSL,
carlosgalvezp and
vbvictor
clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/misc/bool-bitwise-operation.c
Outdated
Show resolved
Hide resolved
localspook
reviewed
Nov 13, 2025
Contributor
localspook
left a comment
localspook
left a comment
There was a problem hiding this comment.
Mostly style/wording comments for now:
clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/misc/bool-bitwise-operation.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/misc/bool-bitwise-operation.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/misc/bool-bitwise-operation.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/misc/bool-bitwise-operation.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/test/clang-tidy/checkers/misc/bool-bitwise-operation.cpp
Outdated
Show resolved
Hide resolved
zwuis
reviewed
Nov 18, 2025
clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.cpp
Outdated
Show resolved
Hide resolved
| return llvm::any_of(Parents, [&](const DynTypedNode &Parent) { | ||
| const auto *S = ignoreParensTowardsTheRoot(&Parent, AC)->get<Stmt>(); | ||
| const auto *ICE = dyn_cast_if_present<ImplicitCastExpr>(S); | ||
| return ICE ? ICE->getType().getDesugaredType(*AC)->isBooleanType() : false; |
Contributor
There was a problem hiding this comment.
getDesugaredType(*AC)
What about using getCanonicalType()?
Author
There was a problem hiding this comment.
Need more time for me to understand the difference between DesugaredType and CanonicalType
clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.cpp
Outdated
Show resolved
Hide resolved
| RootAssignsToBoolean = RootAssignsToBoolean.value_or(true); | ||
| return true; | ||
| } | ||
| if (BinOp->isCompoundAssignmentOp() && IsBooleanLHS) { |
Contributor
There was a problem hiding this comment.
Can we do this check in BoolBitwiseOperationCheck::check()?
Author
There was a problem hiding this comment.
We can't move this compound assignment check outside the isBooleanBitwise function because it would no longer be evaluated during the recursive traversal of nested binary operations, potentially missing cases where compound assignments with boolean operands appear deeper in the expression tree, like this for example:
value |= (flags << 1) | (flags << 2);
Contributor
There was a problem hiding this comment.
The operator precedence of compound assignment is low. It seems that we can
binaryOperator(
unless(...),
+ unless(hasParent(binaryOperator(unless(hasOperatorName(","))))),
);
Author
There was a problem hiding this comment.
Are you suggesting to optimize the matcher excluding comma operator from traversals?
binaryOperator(unless(isExpansionInSystemHeader()),
unless(hasOperatorName(",")),
unless(hasParent(binaryOperator(unless(hasOperatorName(","))))) // ignoring parenExpr
)
Contributor
There was a problem hiding this comment.
binaryOperator(
unless(...),
unless(hasParent(binaryOperator(unless(hasOperatorName(",")))))
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
).bind("binOpRoot")I meant adding this part, so that if there is a compound assignment operator, it must be bound as "binOpRoot", and we can do this check in BoolBitwiseOperationCheck::check().
clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.cpp
Outdated
Show resolved
Hide resolved
zwuis
reviewed
Nov 19, 2025
Comment on lines
+141
to
+146
| Finder->addMatcher( | ||
| binaryOperator(unless(isExpansionInSystemHeader()), | ||
| unless(hasParent(binaryOperator())) // ignoring parenExpr | ||
| ) | ||
| .bind("binOpRoot"), | ||
| this); |
Contributor
There was a problem hiding this comment.
We can write matchers like this to remove assignsToBoolean:
auto M1 = binaryOperator().bind(...);
// as sub-expressions of ImplicitCastExpr or ParenExpr
auto M2 = expr(
anyOf(implicitCastExpr().bind(...), parenExpr()),
ignoringParenImpCasts(M1),
unless(hasParent(anyOf(implicitCastExpr(), parenExpr()))));
// Other cases, including as full expressions
auto M3 = expr(M1, unless(hasParent(anyOf(implicitCastExpr(), parenExpr(), binaryOperator()))));
// I'm not sure if `TK_AsIs` is needed
->addMatcher(expr(unless(isExpansionInSystemHeader()), anyOf(M2, M3)), this);They should be given better names but I have no idea about it.
Author
There was a problem hiding this comment.
Now we can't do it because with TK_IgnoreUnlessSpelledInSource impossible to match implicitCastExpr.
Give me some time to figure out how to write it without TK_IgnoreUnlessSpelledInSource
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
🐧 Linux x64 Test Results
|
vbvictor
reviewed
Nov 21, 2025
clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.cpp
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,9 @@ | |||
| // RUN: %check_clang_tidy %s misc-bool-bitwise-operation %t | |||
Contributor
There was a problem hiding this comment.
Can we merge all small test-cases with same RUN command into one big file
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previous PR had some implementations problems, so I recreated this one.
Two major updates:
-Fixed false-positive in real-world case:
-Implemented diagnostics for structs members
I've tested it on llvm codebase with
-DLLVM_ENABLE_PROJECTS="bolt;clang;clang-tools-extra;compiler-rt;cross-project-tests;libc;libclc;lld;lldb;mlir;openmp;polly"It provided ~20813 warnings and ~653 fixits.
See warnings.log and fixits.diff.txt based on trunk 070f331
Fixes: #40307