Skip to content

Commit 5bc7f73

Browse files
denzor200denzor200
committed
refactor a lot
1 parent 0a1b47f commit 5bc7f73

File tree

4 files changed

+137
-93
lines changed

4 files changed

+137
-93
lines changed

‎clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.cpp‎

Lines changed: 102 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -17,68 +17,46 @@ using namespace clang::ast_matchers;
1717

1818
namespace clang::tidy::misc {
1919

20-
static const Stmt* ignoreParenExpr(const Stmt* S) {
21-
while (const auto* PE = dyn_cast<ParenExpr>(S)) {
22-
S = PE->getSubExpr();
23-
}
24-
return S;
25-
}
26-
27-
template<typename R>
28-
static bool
29-
containsNodeIgnoringParenExpr(R&& children, const Stmt* Node) {
30-
for (const Stmt* Child : children) {
31-
if (ignoreParenExpr(Child) == Node) {
32-
return true;
20+
static const DynTypedNode *ignoreParensTowardsTheRoot(const DynTypedNode *N,
21+
ASTContext *AC) {
22+
if (const auto *S = N->get<Stmt>()) {
23+
if (isa<ParenExpr>(S)) {
24+
auto Parents = AC->getParents(*S);
25+
for (const auto &Parent : Parents) {
26+
return ignoreParensTowardsTheRoot(&Parent, AC);
27+
}
3328
}
3429
}
35-
return false;
30+
return N;
3631
}
3732

38-
static bool isBooleanImplicitCastStmt(const Stmt* S) {
39-
const auto* ICE = dyn_cast<ImplicitCastExpr>(S);
40-
return ICE && ICE->getType()->isBooleanType();
41-
}
33+
static bool assignsToBoolean(const BinaryOperator *BinOp, ASTContext *AC) {
34+
TraversalKindScope RAII(*AC, TK_AsIs);
35+
auto Parents = AC->getParents(*BinOp);
4236

43-
namespace {
44-
AST_MATCHER(BinaryOperator, assignsToBoolean) {
45-
auto Parents = Finder->getASTContext().getParents(Node);
46-
47-
for (const auto& Parent : Parents) {
48-
// Check for Decl parents
49-
if (const auto* D = Parent.template get<Decl>()) {
50-
const Expr* InitExpr = nullptr;
51-
52-
if (const auto* VD = dyn_cast<VarDecl>(D)) {
53-
InitExpr = VD->getInit();
54-
} else if (const auto* FD = dyn_cast<FieldDecl>(D)) {
55-
InitExpr = FD->getInClassInitializer();
56-
} else if (const auto* NTTPD = dyn_cast<NonTypeTemplateParmDecl>(D)) {
37+
for (const auto &Parent : Parents) {
38+
const auto *ParentNoParen = ignoreParensTowardsTheRoot(&Parent, AC);
39+
// Special handling for `template<bool bb=true|1>` cases
40+
if (const auto *D = ParentNoParen->get<Decl>()) {
41+
if (const auto *NTTPD = dyn_cast<NonTypeTemplateParmDecl>(D)) {
5742
if (NTTPD->getType()->isBooleanType()) {
5843
return true;
5944
}
6045
}
61-
62-
if (InitExpr && isBooleanImplicitCastStmt(InitExpr)) {
63-
return true;
64-
}
6546
}
66-
67-
// Check for Stmt parents
68-
if (const auto* S = Parent.template get<Stmt>()) {
69-
for (const Stmt* Child : S->children()) {
70-
if (isBooleanImplicitCastStmt(Child) && containsNodeIgnoringParenExpr(Child->children(), &Node)) {
47+
48+
if (const auto *S = ParentNoParen->get<Stmt>()) {
49+
if (const auto *ICE = dyn_cast<ImplicitCastExpr>(S)) {
50+
if (ICE->getType()->isBooleanType()) {
7151
return true;
7252
}
7353
}
7454
}
7555
}
76-
56+
7757
return false;
7858
}
7959

80-
} // namespace
81-
8260
static const NamedDecl *
8361
getLHSNamedDeclIfCompoundAssign(const BinaryOperator *BO) {
8462
if (BO->isCompoundAssignmentOp()) {
@@ -89,8 +67,6 @@ getLHSNamedDeclIfCompoundAssign(const BinaryOperator *BO) {
8967
return nullptr;
9068
}
9169

92-
constexpr std::array<llvm::StringRef, 4U> OperatorsNames{"|", "&", "|=", "&="};
93-
9470
constexpr std::array<std::pair<llvm::StringRef, llvm::StringRef>, 8U>
9571
OperatorsTransformation{{{"|", "||"},
9672
{"|=", "||"},
@@ -103,13 +79,32 @@ constexpr std::array<std::pair<llvm::StringRef, llvm::StringRef>, 8U>
10379

10480
static llvm::StringRef translate(llvm::StringRef Value) {
10581
for (const auto &[Bitwise, Logical] : OperatorsTransformation) {
106-
if (Value == Bitwise)
82+
if (Value == Bitwise) {
10783
return Logical;
84+
}
10885
}
10986

11087
return {};
11188
}
11289

90+
static bool isBooleanBitwise(const BinaryOperator *BinOp, ASTContext *AC) {
91+
for (const auto &[Bitwise, _] : OperatorsTransformation) {
92+
if (BinOp->getOpcodeStr() == Bitwise) {
93+
const bool hasBooleanOperands = llvm::all_of(
94+
std::array{BinOp->getLHS(), BinOp->getRHS()}, [](const Expr *E) {
95+
return E->IgnoreImpCasts()->getType().getTypePtr()->isBooleanType();
96+
});
97+
if (hasBooleanOperands) {
98+
return true;
99+
}
100+
if (assignsToBoolean(BinOp, AC)) {
101+
return true;
102+
}
103+
}
104+
}
105+
return false;
106+
}
107+
113108
BoolBitwiseOperationCheck::BoolBitwiseOperationCheck(StringRef Name,
114109
ClangTidyContext *Context)
115110
: ClangTidyCheck(Name, Context),
@@ -124,76 +119,67 @@ void BoolBitwiseOperationCheck::storeOptions(
124119

125120
void BoolBitwiseOperationCheck::registerMatchers(MatchFinder *Finder) {
126121
Finder->addMatcher(
127-
binaryOperator(
128-
unless(isExpansionInSystemHeader()),
129-
hasAnyOperatorName(OperatorsNames),
130-
anyOf(
131-
hasOperands(expr(hasType(booleanType())), expr(hasType(booleanType()))),
132-
assignsToBoolean()
133-
),
134-
// isBooleanLikeOperands(),
135-
optionally(hasParent( // to simple implement transformations like
136-
// `a&&b|c` -> `a&&(b||c)`
137-
binaryOperator().bind("p"))))
138-
.bind("op"),
122+
binaryOperator(unless(isExpansionInSystemHeader()),
123+
unless(hasParent(binaryOperator())) // ignoring parenExpr
124+
)
125+
.bind("binOpRoot"),
139126
this);
140127
}
141128

142-
void BoolBitwiseOperationCheck::check(const MatchFinder::MatchResult &Result) {
143-
const auto *MatchedExpr = Result.Nodes.getNodeAs<BinaryOperator>("op");
144-
145-
auto DiagEmitter = [MatchedExpr, this] {
146-
const NamedDecl *ND = getLHSNamedDeclIfCompoundAssign(MatchedExpr);
147-
return diag(MatchedExpr->getOperatorLoc(),
129+
void BoolBitwiseOperationCheck::emitWarningAndChangeOperatorsIfPossible(
130+
const BinaryOperator *BinOp, const BinaryOperator *ParentBinOp,
131+
const clang::SourceManager &SM, clang::ASTContext &Ctx) {
132+
auto DiagEmitter = [BinOp, this] {
133+
const NamedDecl *ND = getLHSNamedDeclIfCompoundAssign(BinOp);
134+
return diag(BinOp->getOperatorLoc(),
148135
"use logical operator '%0' for boolean %select{variable "
149136
"%2|values}1 instead of bitwise operator '%3'")
150-
<< translate(MatchedExpr->getOpcodeStr()) << (ND == nullptr) << ND
151-
<< MatchedExpr->getOpcodeStr();
137+
<< translate(BinOp->getOpcodeStr()) << (ND == nullptr) << ND
138+
<< BinOp->getOpcodeStr();
152139
};
153140

154141
const bool HasVolatileOperand = llvm::any_of(
155-
std::array{MatchedExpr->getLHS(), MatchedExpr->getRHS()},
156-
[](const Expr *E) {
142+
std::array{BinOp->getLHS(), BinOp->getRHS()}, [](const Expr *E) {
157143
return E->IgnoreImpCasts()->getType().isVolatileQualified();
158144
});
159145
if (HasVolatileOperand)
160146
return static_cast<void>(DiagEmitter());
161147

162-
const bool HasSideEffects = MatchedExpr->getRHS()->HasSideEffects(
163-
*Result.Context, /*IncludePossibleEffects=*/!StrictMode);
148+
const bool HasSideEffects = BinOp->getRHS()->HasSideEffects(
149+
Ctx, /*IncludePossibleEffects=*/!StrictMode);
164150
if (HasSideEffects)
165151
return static_cast<void>(DiagEmitter());
166152

167-
SourceLocation Loc = MatchedExpr->getOperatorLoc();
153+
SourceLocation Loc = BinOp->getOperatorLoc();
168154

169155
if (Loc.isInvalid() || Loc.isMacroID())
170156
return static_cast<void>(IgnoreMacros || DiagEmitter());
171157

172-
Loc = Result.SourceManager->getSpellingLoc(Loc);
158+
Loc = SM.getSpellingLoc(Loc);
173159
if (Loc.isInvalid() || Loc.isMacroID())
174160
return static_cast<void>(IgnoreMacros || DiagEmitter());
175161

176162
const CharSourceRange TokenRange = CharSourceRange::getTokenRange(Loc);
177163
if (TokenRange.isInvalid())
178164
return static_cast<void>(IgnoreMacros || DiagEmitter());
179165

180-
const StringRef FixSpelling = translate(Lexer::getSourceText(
181-
TokenRange, *Result.SourceManager, Result.Context->getLangOpts()));
166+
const StringRef FixSpelling =
167+
translate(Lexer::getSourceText(TokenRange, SM, Ctx.getLangOpts()));
182168

183169
if (FixSpelling.empty())
184170
return static_cast<void>(DiagEmitter());
185171

186172
FixItHint InsertEqual;
187-
if (MatchedExpr->isCompoundAssignmentOp()) {
173+
if (BinOp->isCompoundAssignmentOp()) {
188174
const auto *DeclRefLHS =
189-
dyn_cast<DeclRefExpr>(MatchedExpr->getLHS()->IgnoreImpCasts());
175+
dyn_cast<DeclRefExpr>(BinOp->getLHS()->IgnoreImpCasts());
190176
if (!DeclRefLHS)
191177
return static_cast<void>(DiagEmitter());
192178
const SourceLocation LocLHS = DeclRefLHS->getEndLoc();
193179
if (LocLHS.isInvalid() || LocLHS.isMacroID())
194180
return static_cast<void>(IgnoreMacros || DiagEmitter());
195-
const SourceLocation InsertLoc = clang::Lexer::getLocForEndOfToken(
196-
LocLHS, 0, *Result.SourceManager, Result.Context->getLangOpts());
181+
const SourceLocation InsertLoc =
182+
clang::Lexer::getLocForEndOfToken(LocLHS, 0, SM, Ctx.getLangOpts());
197183
if (InsertLoc.isInvalid() || InsertLoc.isMacroID())
198184
return static_cast<void>(IgnoreMacros || DiagEmitter());
199185
InsertEqual = FixItHint::CreateInsertion(
@@ -202,27 +188,25 @@ void BoolBitwiseOperationCheck::check(const MatchFinder::MatchResult &Result) {
202188

203189
auto ReplaceOperator = FixItHint::CreateReplacement(TokenRange, FixSpelling);
204190

205-
const auto *Parent = Result.Nodes.getNodeAs<BinaryOperator>("p");
206191
std::optional<BinaryOperatorKind> ParentOpcode;
207-
if (Parent)
208-
ParentOpcode = Parent->getOpcode();
192+
if (ParentBinOp)
193+
ParentOpcode = ParentBinOp->getOpcode();
209194

210-
const auto *RHS =
211-
dyn_cast<BinaryOperator>(MatchedExpr->getRHS()->IgnoreImpCasts());
195+
const auto *RHS = dyn_cast<BinaryOperator>(BinOp->getRHS()->IgnoreImpCasts());
212196
std::optional<BinaryOperatorKind> RHSOpcode;
213197
if (RHS)
214198
RHSOpcode = RHS->getOpcode();
215199

216200
const Expr *SurroundedExpr = nullptr;
217-
if ((MatchedExpr->getOpcode() == BO_Or && ParentOpcode == BO_LAnd) ||
218-
(MatchedExpr->getOpcode() == BO_And &&
201+
if ((BinOp->getOpcode() == BO_Or && ParentOpcode == BO_LAnd) ||
202+
(BinOp->getOpcode() == BO_And &&
219203
llvm::is_contained({BO_Xor, BO_Or}, ParentOpcode))) {
220-
const Expr *Side = Parent->getLHS()->IgnoreParenImpCasts() == MatchedExpr
221-
? Parent->getLHS()
222-
: Parent->getRHS();
204+
const Expr *Side = ParentBinOp->getLHS()->IgnoreParenImpCasts() == BinOp
205+
? ParentBinOp->getLHS()
206+
: ParentBinOp->getRHS();
223207
SurroundedExpr = Side->IgnoreImpCasts();
224-
assert(SurroundedExpr->IgnoreParens() == MatchedExpr);
225-
} else if (MatchedExpr->getOpcode() == BO_AndAssign && RHSOpcode == BO_LOr)
208+
assert(SurroundedExpr->IgnoreParens() == BinOp);
209+
} else if (BinOp->getOpcode() == BO_AndAssign && RHSOpcode == BO_LOr)
226210
SurroundedExpr = RHS;
227211

228212
if (SurroundedExpr && isa<ParenExpr>(SurroundedExpr))
@@ -233,8 +217,7 @@ void BoolBitwiseOperationCheck::check(const MatchFinder::MatchResult &Result) {
233217
if (SurroundedExpr) {
234218
const SourceLocation InsertFirstLoc = SurroundedExpr->getBeginLoc();
235219
const SourceLocation InsertSecondLoc = clang::Lexer::getLocForEndOfToken(
236-
SurroundedExpr->getEndLoc(), 0, *Result.SourceManager,
237-
Result.Context->getLangOpts());
220+
SurroundedExpr->getEndLoc(), 0, SM, Ctx.getLangOpts());
238221
if (InsertFirstLoc.isInvalid() || InsertFirstLoc.isMacroID() ||
239222
InsertSecondLoc.isInvalid() || InsertSecondLoc.isMacroID())
240223
return static_cast<void>(IgnoreMacros || DiagEmitter());
@@ -246,4 +229,30 @@ void BoolBitwiseOperationCheck::check(const MatchFinder::MatchResult &Result) {
246229
<< InsertBrace2;
247230
}
248231

232+
void BoolBitwiseOperationCheck::visitBinaryTreesNode(
233+
const BinaryOperator *BinOp, const BinaryOperator *ParentBinOp,
234+
const clang::SourceManager &SM, clang::ASTContext &Ctx) {
235+
if (!BinOp) {
236+
return;
237+
}
238+
239+
if (isBooleanBitwise(BinOp, &Ctx)) {
240+
emitWarningAndChangeOperatorsIfPossible(BinOp, ParentBinOp, SM, Ctx);
241+
}
242+
243+
visitBinaryTreesNode(
244+
dyn_cast<BinaryOperator>(BinOp->getLHS()->IgnoreParenImpCasts()), BinOp,
245+
SM, Ctx);
246+
visitBinaryTreesNode(
247+
dyn_cast<BinaryOperator>(BinOp->getRHS()->IgnoreParenImpCasts()), BinOp,
248+
SM, Ctx);
249+
}
250+
251+
void BoolBitwiseOperationCheck::check(const MatchFinder::MatchResult &Result) {
252+
const auto *binOpRoot = Result.Nodes.getNodeAs<BinaryOperator>("binOpRoot");
253+
const SourceManager &SM = *Result.SourceManager;
254+
ASTContext &Ctx = *Result.Context;
255+
visitBinaryTreesNode(binOpRoot, nullptr, SM, Ctx);
256+
}
257+
249258
} // namespace clang::tidy::misc

‎clang-tools-extra/clang-tidy/misc/BoolBitwiseOperationCheck.h‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,15 @@ class BoolBitwiseOperationCheck : public ClangTidyCheck {
3232
return TK_IgnoreUnlessSpelledInSource;
3333
}
3434

35+
private:
36+
void emitWarningAndChangeOperatorsIfPossible(
37+
const BinaryOperator *BinOp, const BinaryOperator *ParentBinOp,
38+
const clang::SourceManager &SM, clang::ASTContext &Ctx);
39+
void visitBinaryTreesNode(const BinaryOperator *BinOp,
40+
const BinaryOperator *ParentBinOp,
41+
const clang::SourceManager &SM,
42+
clang::ASTContext &Ctx);
43+
3544
private:
3645
bool StrictMode;
3746
bool IgnoreMacros;

‎clang-tools-extra/test/clang-tidy/checkers/misc/bool-bitwise-operation-nontraditional.cpp‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,9 @@ void bad_side_effects() {
125125
void bad_side_effects_volatile() {
126126
bool a = true;
127127
volatile bool b = false;
128+
bool c = true;
129+
bool r;
130+
128131
a bitor b;
129132
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use logical operator '||' for boolean values instead of bitwise operator '|' [misc-bool-bitwise-operation]
130133
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
@@ -138,6 +141,16 @@ void bad_side_effects_volatile() {
138141
a and_eq b;
139142
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use logical operator '&&' for boolean variable 'a' instead of bitwise operator '&=' [misc-bool-bitwise-operation]
140143
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
144+
145+
r = (a bitor c) bitor b;
146+
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use logical operator '||' for boolean values instead of bitwise operator '|' [misc-bool-bitwise-operation]
147+
// CHECK-MESSAGES: :[[@LINE-2]]:21: warning: use logical operator '||' for boolean values instead of bitwise operator '|' [misc-bool-bitwise-operation]
148+
// CHECK-FIXES: r = (a or c) bitor b;
149+
150+
r = a bitor c bitor b;
151+
// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use logical operator '||' for boolean values instead of bitwise operator '|' [misc-bool-bitwise-operation]
152+
// CHECK-MESSAGES: :[[@LINE-2]]:19: warning: use logical operator '||' for boolean values instead of bitwise operator '|' [misc-bool-bitwise-operation]
153+
// CHECK-FIXES: r = a or c bitor b;
141154
}
142155

143156
void bad_with_priors() {

‎clang-tools-extra/test/clang-tidy/checkers/misc/bool-bitwise-operation.cpp‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,9 @@ void bad_side_effects() {
125125
void bad_side_effects_volatile() {
126126
bool a = true;
127127
volatile bool b = false;
128+
bool c = true;
129+
bool r;
130+
128131
a | b;
129132
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use logical operator '||' for boolean values instead of bitwise operator '|' [misc-bool-bitwise-operation]
130133
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
@@ -138,6 +141,16 @@ void bad_side_effects_volatile() {
138141
a &= b;
139142
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use logical operator '&&' for boolean variable 'a' instead of bitwise operator '&=' [misc-bool-bitwise-operation]
140143
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
144+
145+
r = (a | c) | b;
146+
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: use logical operator '||' for boolean values instead of bitwise operator '|' [misc-bool-bitwise-operation]
147+
// CHECK-MESSAGES: :[[@LINE-2]]:17: warning: use logical operator '||' for boolean values instead of bitwise operator '|' [misc-bool-bitwise-operation]
148+
// CHECK-FIXES: r = (a || c) | b;
149+
150+
r = a | c | b;
151+
// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use logical operator '||' for boolean values instead of bitwise operator '|' [misc-bool-bitwise-operation]
152+
// CHECK-MESSAGES: :[[@LINE-2]]:15: warning: use logical operator '||' for boolean values instead of bitwise operator '|' [misc-bool-bitwise-operation]
153+
// CHECK-FIXES: r = a || c | b;
141154
}
142155

143156
void bad_with_priors() {

0 commit comments

Comments
 (0)