[clang-tools-extra] 709112b - [clang-tidy] false-positive for bugprone-redundant-branch-condition in case of passed-by-ref params
Zinovy Nis via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 11 10:10:15 PST 2020
Author: Zinovy Nis
Date: 2020-12-11T21:09:51+03:00
New Revision: 709112bce4424a5436f0bb699c62b3fbc837fbb6
URL: https://github.com/llvm/llvm-project/commit/709112bce4424a5436f0bb699c62b3fbc837fbb6
DIFF: https://github.com/llvm/llvm-project/commit/709112bce4424a5436f0bb699c62b3fbc837fbb6.diff
LOG: [clang-tidy] false-positive for bugprone-redundant-branch-condition in case of passed-by-ref params
Inspired by discussion in https://reviews.llvm.org/D91037
Differential Revision: https://reviews.llvm.org/D91495
Added:
Modified:
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
index abb8e8be9b2f..2b0d9630527b 100644
--- a/clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -23,15 +23,21 @@ namespace bugprone {
static const char CondVarStr[] = "cond_var";
static const char OuterIfStr[] = "outer_if";
static const char InnerIfStr[] = "inner_if";
+static const char OuterIfVar1Str[] = "outer_if_var1";
+static const char OuterIfVar2Str[] = "outer_if_var2";
+static const char InnerIfVar1Str[] = "inner_if_var1";
+static const char InnerIfVar2Str[] = "inner_if_var2";
static const char FuncStr[] = "func";
-/// Returns whether `Var` is changed in `S` before `NextS`.
-static bool isChangedBefore(const Stmt *S, const Stmt *NextS,
+/// Returns whether `Var` is changed in range (`PrevS`..`NextS`).
+static bool isChangedBefore(const Stmt *S, const Stmt *NextS, const Stmt *PrevS,
const VarDecl *Var, ASTContext *Context) {
ExprMutationAnalyzer MutAn(*S, *Context);
const auto &SM = Context->getSourceManager();
const Stmt *MutS = MutAn.findMutation(Var);
return MutS &&
+ SM.isBeforeInTranslationUnit(PrevS->getEndLoc(),
+ MutS->getBeginLoc()) &&
SM.isBeforeInTranslationUnit(MutS->getEndLoc(), NextS->getBeginLoc());
}
@@ -43,19 +49,22 @@ void RedundantBranchConditionCheck::registerMatchers(MatchFinder *Finder) {
Finder->addMatcher(
ifStmt(
hasCondition(ignoringParenImpCasts(anyOf(
- declRefExpr(hasDeclaration(ImmutableVar)),
+ declRefExpr(hasDeclaration(ImmutableVar)).bind(OuterIfVar1Str),
binaryOperator(hasOperatorName("&&"),
- hasEitherOperand(ignoringParenImpCasts(declRefExpr(
- hasDeclaration(ImmutableVar)))))))),
+ hasEitherOperand(ignoringParenImpCasts(
+ declRefExpr(hasDeclaration(ImmutableVar))
+ .bind(OuterIfVar2Str))))))),
hasThen(hasDescendant(
ifStmt(hasCondition(ignoringParenImpCasts(
- anyOf(declRefExpr(hasDeclaration(
- varDecl(equalsBoundNode(CondVarStr)))),
+ anyOf(declRefExpr(hasDeclaration(varDecl(
+ equalsBoundNode(CondVarStr))))
+ .bind(InnerIfVar1Str),
binaryOperator(
hasAnyOperatorName("&&", "||"),
hasEitherOperand(ignoringParenImpCasts(
declRefExpr(hasDeclaration(varDecl(
- equalsBoundNode(CondVarStr)))))))))))
+ equalsBoundNode(CondVarStr))))
+ .bind(InnerIfVar2Str))))))))
.bind(InnerIfStr))),
forFunction(functionDecl().bind(FuncStr)))
.bind(OuterIfStr),
@@ -69,15 +78,32 @@ void RedundantBranchConditionCheck::check(const MatchFinder::MatchResult &Result
const auto *CondVar = Result.Nodes.getNodeAs<VarDecl>(CondVarStr);
const auto *Func = Result.Nodes.getNodeAs<FunctionDecl>(FuncStr);
+ const DeclRefExpr *OuterIfVar, *InnerIfVar;
+ if (const auto *Inner = Result.Nodes.getNodeAs<DeclRefExpr>(InnerIfVar1Str))
+ InnerIfVar = Inner;
+ else
+ InnerIfVar = Result.Nodes.getNodeAs<DeclRefExpr>(InnerIfVar2Str);
+ if (const auto *Outer = Result.Nodes.getNodeAs<DeclRefExpr>(OuterIfVar1Str))
+ OuterIfVar = Outer;
+ else
+ OuterIfVar = Result.Nodes.getNodeAs<DeclRefExpr>(OuterIfVar2Str);
+
+ if (OuterIfVar && InnerIfVar) {
+ if (isChangedBefore(OuterIf->getThen(), InnerIfVar, OuterIfVar, CondVar,
+ Result.Context))
+ return;
+
+ if (isChangedBefore(OuterIf->getCond(), InnerIfVar, OuterIfVar, CondVar,
+ Result.Context))
+ return;
+ }
+
// If the variable has an alias then it can be changed by that alias as well.
// FIXME: could potentially support tracking pointers and references in the
// future to improve catching true positives through aliases.
if (hasPtrOrReferenceInFunc(Func, CondVar))
return;
- if (isChangedBefore(OuterIf->getThen(), InnerIf, CondVar, Result.Context))
- return;
-
auto Diag = diag(InnerIf->getBeginLoc(), "redundant condition %0") << CondVar;
// For standalone condition variables and for "or" binary operations we simply
@@ -123,7 +149,7 @@ void RedundantBranchConditionCheck::check(const MatchFinder::MatchResult &Result
CharSourceRange::getTokenRange(IfBegin, IfEnd));
}
- // For comound statements also remove the right brace at the end.
+ // For compound statements also remove the right brace at the end.
if (isa<CompoundStmt>(Body))
Diag << FixItHint::CreateRemoval(
CharSourceRange::getTokenRange(Body->getEndLoc(), Body->getEndLoc()));
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
index dd001e836477..e59d66462899 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -6,7 +6,8 @@ extern unsigned fireFighters;
bool isBurning();
bool isReallyBurning();
bool isCollapsing();
-void tryToExtinguish(bool&);
+bool tryToExtinguish(bool&);
+bool tryToExtinguishByVal(bool);
void tryPutFireOut();
bool callTheFD();
void scream();
@@ -948,6 +949,30 @@ void negative_indirect2_outer_and_rhs_inner_or_rhs() {
}
}
+void negative_by_ref(bool onFire) {
+ if (tryToExtinguish(onFire) && onFire) {
+ if (tryToExtinguish(onFire) && onFire) {
+ // NO-MESSAGE: fire may have been extinguished
+ scream();
+ }
+ }
+}
+
+void negative_by_val(bool onFire) {
+ if (tryToExtinguishByVal(onFire) && onFire) {
+ if (tryToExtinguish(onFire) && onFire) {
+ // NO-MESSAGE: fire may have been extinguished
+ scream();
+ }
+ }
+ if (tryToExtinguish(onFire) && onFire) {
+ if (tryToExtinguishByVal(onFire) && onFire) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'onFire' [bugprone-redundant-branch-condition]
+ scream();
+ }
+ }
+}
+
void negative_reassigned() {
bool onFire = isBurning();
if (onFire) {
@@ -1077,9 +1102,9 @@ void positive_comma_after_condition() {
int positive_expr_with_cleanups() {
class RetT {
public:
- RetT(const int _code) : code_(_code) {}
- bool Ok() const { return code_ == 0; }
- static RetT Test(bool &_isSet) { return 0; }
+ RetT(const int code);
+ bool Ok() const;
+ static RetT Test(bool isSet);
private:
int code_;
More information about the cfe-commits
mailing list