[clang-tidy] false-positive for bugprone-redundant-branch-condition in case of passed...
authorZinovy Nis <zinovy.nis@gmail.com>
Sun, 15 Nov 2020 15:09:18 +0000 (18:09 +0300)
committerZinovy Nis <zinovy.nis@gmail.com>
Fri, 11 Dec 2020 18:09:51 +0000 (21:09 +0300)
Inspired by discussion in https://reviews.llvm.org/D91037

Differential Revision: https://reviews.llvm.org/D91495

clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp

index abb8e8b..2b0d963 100644 (file)
@@ -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()));
index dd001e8..e59d664 100644 (file)
@@ -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_;