Skip to content

Commit 3b8a308

Browse files
denzor200denzor200
committed
review
1 parent 53bd509 commit 3b8a308

File tree

4 files changed

+44
-92
lines changed

4 files changed

+44
-92
lines changed

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

Lines changed: 19 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
#include "clang/ASTMatchers/ASTMatchFinder.h"
1111
#include "clang/Lex/Lexer.h"
1212
#include "llvm/ADT/ScopeExit.h"
13-
#include "llvm/ADT/Twine.h"
1413
#include <array>
1514
#include <optional>
1615
#include <utility>
@@ -19,16 +18,14 @@ using namespace clang::ast_matchers;
1918

2019
namespace clang::tidy::performance {
2120

22-
static std::string tryPrintVariable(const BinaryOperator *BO) {
21+
static const NamedDecl *
22+
getLHSNamedDeclIfCompoundAssign(const BinaryOperator *BO) {
2323
if (BO->isCompoundAssignmentOp()) {
2424
const auto *DelcRefLHS =
2525
dyn_cast<DeclRefExpr>(BO->getLHS()->IgnoreImpCasts());
26-
if (DelcRefLHS)
27-
return ("variable '" +
28-
llvm::Twine(DelcRefLHS->getDecl()->getNameAsString()) + "'")
29-
.str();
26+
return DelcRefLHS ? DelcRefLHS->getDecl() : nullptr;
3027
}
31-
return "values";
28+
return nullptr;
3229
}
3330

3431
static bool hasExplicitParentheses(const Expr *E, const SourceManager &SM,
@@ -52,19 +49,6 @@ static bool hasExplicitParentheses(const Expr *E, const SourceManager &SM,
5249
(NextTok && NextTok->is(tok::r_paren));
5350
}
5451

55-
template <typename AstNode>
56-
static bool isInTemplateFunction(const AstNode *AN, ASTContext &Context) {
57-
DynTypedNodeList Parents = Context.getParents(*AN);
58-
for (const DynTypedNode &Parent : Parents) {
59-
if (const auto *FD = Parent.template get<FunctionDecl>())
60-
return FD->isTemplateInstantiation() ||
61-
FD->getTemplatedKind() != FunctionDecl::TK_NonTemplate;
62-
if (const auto *S = Parent.template get<Stmt>())
63-
return isInTemplateFunction(S, Context);
64-
}
65-
return false;
66-
}
67-
6852
constexpr std::array<std::pair<llvm::StringRef, llvm::StringRef>, 8U>
6953
OperatorsTransformation{{{"|", "||"},
7054
{"|=", "||"},
@@ -101,9 +85,9 @@ void BoolBitwiseOperationCheck::registerMatchers(MatchFinder *Finder) {
10185
binaryOperator(
10286
unless(isExpansionInSystemHeader()),
10387
hasAnyOperatorName("|", "&", "|=", "&="),
104-
hasEitherOperand(expr(ignoringImpCasts(hasType(booleanType())))),
105-
optionally(hasAncestor( // to simple implement transformations like
106-
// `a&&b|c` -> `a&&(b||c)`
88+
hasEitherOperand(expr(hasType(booleanType()))),
89+
optionally(hasParent( // to simple implement transformations like
90+
// `a&&b|c` -> `a&&(b||c)`
10791
binaryOperator().bind("p"))))
10892
.bind("op"),
10993
this);
@@ -113,25 +97,27 @@ void BoolBitwiseOperationCheck::check(const MatchFinder::MatchResult &Result) {
11397
const auto *MatchedExpr = Result.Nodes.getNodeAs<BinaryOperator>("op");
11498

11599
auto DiagEmitterProc = [MatchedExpr, this] {
100+
const NamedDecl *ND = getLHSNamedDeclIfCompoundAssign(MatchedExpr);
116101
return diag(MatchedExpr->getOperatorLoc(),
117-
"use logical operator '%0' for boolean %1 instead of "
118-
"bitwise operator '%2'")
119-
<< translate(MatchedExpr->getOpcodeStr())
120-
<< tryPrintVariable(MatchedExpr) << MatchedExpr->getOpcodeStr();
102+
"use logical operator '%0' for boolean %select{variable "
103+
"%2|values}1 instead of "
104+
"bitwise operator '%3'")
105+
<< translate(MatchedExpr->getOpcodeStr()) << (ND == nullptr) << ND
106+
<< MatchedExpr->getOpcodeStr();
121107
};
122108
auto DiagEmitter = llvm::make_scope_exit(DiagEmitterProc);
123109

124-
if (isInTemplateFunction(MatchedExpr, *Result.Context))
125-
return;
126-
127110
const bool HasVolatileOperand = llvm::any_of(
128111
std::array{MatchedExpr->getLHS(), MatchedExpr->getRHS()},
129112
[](const Expr *E) {
130113
return E->IgnoreImpCasts()->getType().isVolatileQualified();
131114
});
115+
if (HasVolatileOperand)
116+
return;
117+
132118
const bool HasSideEffects =
133119
MatchedExpr->getRHS()->HasSideEffects(*Result.Context, StrictMode);
134-
if (HasVolatileOperand || HasSideEffects)
120+
if (HasSideEffects)
135121
return;
136122

137123
SourceLocation Loc = MatchedExpr->getOperatorLoc();
@@ -189,16 +175,11 @@ void BoolBitwiseOperationCheck::check(const MatchFinder::MatchResult &Result) {
189175
auto ReplaceOperator = FixItHint::CreateReplacement(TokenRange, FixSpelling);
190176

191177
std::optional<BinaryOperatorKind> ParentOpcode;
192-
if (const auto *Parent = Result.Nodes.getNodeAs<BinaryOperator>("p");
193-
Parent && llvm::any_of(std::array{Parent->getLHS(), Parent->getRHS()},
194-
[&](const Expr *E) {
195-
return dyn_cast<BinaryOperator>(
196-
E->IgnoreImpCasts()) == MatchedExpr;
197-
}))
178+
if (const auto *Parent = Result.Nodes.getNodeAs<BinaryOperator>("p"); Parent)
198179
ParentOpcode = Parent->getOpcode();
199180

200181
const auto *RHS =
201-
dyn_cast<BinaryOperator>(MatchedExpr->getRHS()->IgnoreParenCasts());
182+
dyn_cast<BinaryOperator>(MatchedExpr->getRHS()->IgnoreImpCasts());
202183
std::optional<BinaryOperatorKind> RHSOpcode;
203184
if (RHS)
204185
RHSOpcode = RHS->getOpcode();

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,9 @@ class BoolBitwiseOperationCheck : public ClangTidyCheck {
2828
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
2929
return LangOpts.CPlusPlus || LangOpts.C99;
3030
}
31+
std::optional<TraversalKind> getCheckTraversalKind() const override {
32+
return TK_IgnoreUnlessSpelledInSource;
33+
}
3134

3235
private:
3336
bool StrictMode;

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

Lines changed: 11 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -374,40 +374,24 @@ void good_in_unreachable_template(T a, T b) {
374374
template<typename T>
375375
int bad_in_template(T a, T b) {
376376
bool c = false;
377-
a bitor b;
378-
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use logical operator '||' for boolean values instead of bitwise operator '|' [performance-bool-bitwise-operation]
379-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
380-
a bitand b;
381-
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use logical operator '&&' for boolean values instead of bitwise operator '&' [performance-bool-bitwise-operation]
382-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
383-
a or_eq b;
384-
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use logical operator '||' for boolean variable 'a' instead of bitwise operator '|=' [performance-bool-bitwise-operation]
385-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
386-
a and_eq b;
387-
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use logical operator '&&' for boolean variable 'a' instead of bitwise operator '&=' [performance-bool-bitwise-operation]
388-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
389-
c and_eq a;
390-
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use logical operator '&&' for boolean variable 'c' instead of bitwise operator '&=' [performance-bool-bitwise-operation]
391-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
377+
// FIXME: at least warning should be provided in these cases
378+
// a bitor b;
379+
// a bitand b;
380+
// a or_eq b;
381+
// a and_eq b;
382+
// c and_eq a;
392383
return 0;
393384
}
394385

395386
template<typename T>
396387
int bad_in_template_lamnda_captured(T a, T b) {
397388
[=] mutable {
398389
bool c = false;
399-
a bitor b;
400-
// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use logical operator '||' for boolean values instead of bitwise operator '|' [performance-bool-bitwise-operation]
401-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
402-
a bitand b;
403-
// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use logical operator '&&' for boolean values instead of bitwise operator '&' [performance-bool-bitwise-operation]
404-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
405-
a or_eq b;
406-
// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use logical operator '||' for boolean variable 'a' instead of bitwise operator '|=' [performance-bool-bitwise-operation]
407-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
408-
b and_eq a;
409-
// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use logical operator '&&' for boolean variable 'b' instead of bitwise operator '&=' [performance-bool-bitwise-operation]
410-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
390+
// FIXME: at least warning should be provided in these cases
391+
// a bitor b;
392+
// a bitand b;
393+
// a or_eq b;
394+
// b and_eq a;
411395
}();
412396
return 0;
413397
}

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

Lines changed: 11 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -374,40 +374,24 @@ void good_in_unreachable_template(T a, T b) {
374374
template<typename T>
375375
int bad_in_template(T a, T b) {
376376
bool c = false;
377-
a | b;
378-
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use logical operator '||' for boolean values instead of bitwise operator '|' [performance-bool-bitwise-operation]
379-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
380-
a & b;
381-
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use logical operator '&&' for boolean values instead of bitwise operator '&' [performance-bool-bitwise-operation]
382-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
383-
a |= b;
384-
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use logical operator '||' for boolean variable 'a' instead of bitwise operator '|=' [performance-bool-bitwise-operation]
385-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
386-
a &= b;
387-
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use logical operator '&&' for boolean variable 'a' instead of bitwise operator '&=' [performance-bool-bitwise-operation]
388-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
389-
c &= a;
390-
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use logical operator '&&' for boolean variable 'c' instead of bitwise operator '&=' [performance-bool-bitwise-operation]
391-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
377+
// FIXME: at least warning should be provided in these cases
378+
// a | b;
379+
// a & b;
380+
// a |= b;
381+
// a &= b;
382+
// c &= a;
392383
return 0;
393384
}
394385

395386
template<typename T>
396387
int bad_in_template_lamnda_captured(T a, T b) {
397388
[=] mutable {
398389
bool c = false;
399-
a | b;
400-
// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use logical operator '||' for boolean values instead of bitwise operator '|' [performance-bool-bitwise-operation]
401-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
402-
a & b;
403-
// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use logical operator '&&' for boolean values instead of bitwise operator '&' [performance-bool-bitwise-operation]
404-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
405-
a |= b;
406-
// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use logical operator '||' for boolean variable 'a' instead of bitwise operator '|=' [performance-bool-bitwise-operation]
407-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
408-
b &= a;
409-
// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: use logical operator '&&' for boolean variable 'b' instead of bitwise operator '&=' [performance-bool-bitwise-operation]
410-
// CHECK-MESSAGES-NOT: :[[@LINE-2]]:{{.*}}: note: FIX-IT applied suggested code changes
390+
// FIXME: at least warning should be provided in these cases
391+
// a | b;
392+
// a & b;
393+
// a |= b;
394+
// b &= a;
411395
}();
412396
return 0;
413397
}

0 commit comments

Comments
 (0)