Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp =================================================================== --- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp +++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp @@ -23,15 +23,21 @@ 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 @@ 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 @@ const auto *CondVar = Result.Nodes.getNodeAs(CondVarStr); const auto *Func = Result.Nodes.getNodeAs(FuncStr); + const DeclRefExpr *OuterIfVar, *InnerIfVar; + if (const auto *Inner = Result.Nodes.getNodeAs(InnerIfVar1Str)) + InnerIfVar = Inner; + else + InnerIfVar = Result.Nodes.getNodeAs(InnerIfVar2Str); + if (const auto *Outer = Result.Nodes.getNodeAs(OuterIfVar1Str)) + OuterIfVar = Outer; + else + OuterIfVar = Result.Nodes.getNodeAs(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 @@ 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(Body)) Diag << FixItHint::CreateRemoval( CharSourceRange::getTokenRange(Body->getEndLoc(), Body->getEndLoc())); Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp =================================================================== --- clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp +++ clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp @@ -6,7 +6,8 @@ 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_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 @@ 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_;