[clang-tidy] false-positive for bugprone-redundant-branch-condition in case of passed-by-ref params

[zinovy.nis]

Summary

Inspired by discussion in https://reviews.llvm.org/D91037

[zinovy.nis]

Extend the context.

[aaron.ballman]

Thank you for working on this!

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

10

Did you mean for the parameter to be a reference?

963

FWIW, I would expect this one to be diagnosed if the call really was byval instead of byref.

968

Another test that would be interesting is:

if (tryToExtinguish(isSet) && isSet) {
  if (tryToExtinguishByVal(isSet) && isSet) { // Dupe check of isSet should be diagnosed
    scream();
  }
}

if (tryToExtinguishByVal(isSet) && isSet) {
  if (tryToExtinguish(isSet) && isSet) { // Ok
    scream();
  }
}

[zinovy.nis]

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

968

Nice catch! Thanks. Looks like isChangedBefore is too rough and must be refined

[zinovy.nis]

Handle ref & val mixed cases.

[zinovy.nis]

What do you think of it?

[aaron.ballman]

clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp

35

I think this comment should be hoisted above to update the comment on line 32.

49

80-col limit issue -- you should run the patch through clang-format.

77

const auto *, but I think this would be better expressed as:

const DeclRefExpr *OuterIfVar, *InnerIfVar;
if (const auto *Outer = Result.Nodes.getNodeAs<DeclRefExpr>(OuterIfVar1Str))
  OuterIfVar = Outer;
else
  OuterIfVar = Result.Nodes.getNodeAs<DeclRefExpr>(OuterIfVar2Str);

// Similar for InnerIfVar

[zinovy.nis]

Fixed remarks from Aaron.

[aaron.ballman]

LGTM, thank you for the fix!

clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp

 
 /// Returns whether `Var` is changed in `S` before `NextS`.
 static bool isChangedBefore(const Stmt *S, const Stmt *NextS,
                             const VarDecl *Var, ASTContext *Context) {
   ExprMutationAnalyzer MutAn(*S, *Context);
   const auto &SM = Context->getSourceManager();
   const Stmt *MutS = MutAn.findMutation(Var);
   return MutS &&
          SM.isBeforeInTranslationUnit(MutS->getEndLoc(), NextS->getBeginLoc());
 }
 
 void RedundantBranchConditionCheck::registerMatchers(MatchFinder *Finder) {
   const auto ImmutableVar =
       varDecl(anyOf(parmVarDecl(), hasLocalStorage()), hasType(isInteger()),
               unless(hasType(isVolatileQualified())))
           .bind(CondVarStr);
   Finder->addMatcher(
       ifStmt(
           hasCondition(ignoringParenImpCasts(anyOf(
               declRefExpr(hasDeclaration(ImmutableVar)),
               binaryOperator(hasOperatorName("&&"),
                              hasEitherOperand(ignoringParenImpCasts(declRefExpr(
                                  hasDeclaration(ImmutableVar)))))))),
           hasThen(hasDescendant(
               ifStmt(hasCondition(ignoringParenImpCasts(
                          anyOf(declRefExpr(hasDeclaration(
                                    varDecl(equalsBoundNode(CondVarStr)))),
                                binaryOperator(
                                    hasAnyOperatorName("&&", "||"),
                                    hasEitherOperand(ignoringParenImpCasts(
                                        declRefExpr(hasDeclaration(varDecl(
void RedundantBranchConditionCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *CondVar = Result.Nodes.getNodeAs<VarDecl>(CondVarStr);
   const auto *Func = Result.Nodes.getNodeAs<FunctionDecl>(FuncStr);
 
   // 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->getCond(), InnerIf, CondVar, Result.Context))
+    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
   // remove the inner `if`.
   const auto *BinOpCond =
if (OtherSide && OtherSide->HasSideEffects(*Result.Context)) {
            << FixItHint::CreateInsertion(AfterOtherSide, ";")
            << FixItHint::CreateRemoval(
                   CharSourceRange::getTokenRange(AfterOtherSide, IfEnd));
     } else {
       Diag << FixItHint::CreateRemoval(
           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()));
 
     // For "and" binary operations we remove the "and" operation with the
     // condition variable from the inner if.
   } else {
     const auto *CondOp =

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

 // RUN: %check_clang_tidy %s bugprone-redundant-branch-condition %t
 
 extern unsigned peopleInTheBuilding;
 extern unsigned fireFighters;
 
 bool isBurning();
 bool isReallyBurning();
 bool isCollapsing();
-void tryToExtinguish(bool&);
+bool tryToExtinguish(bool&);
+bool tryToExtinguishByVal(bool &);
 void tryPutFireOut();
 bool callTheFD();
 void scream();
 
 bool someOtherCondition();
 
 //===--- Basic Positives --------------------------------------------------===//
 
if (someOtherCondition()) {
       if (isCollapsing() || onFire) {
         // NO-MESSAGE: fire may have been extinguished
         scream();
       }
     }
   }
 }
 
+void negative_by_ref(bool isSet) {
+  if (tryToExtinguish(isSet) && isSet) {
+    if (tryToExtinguish(isSet) && isSet) {
+      // NO-MESSAGE: fire may have been extinguished
+      scream();
+    }
+  }
+}
+
+void negative_by_val(bool isSet) {
+  if (tryToExtinguishByVal(isSet) && isSet) {
+    if (tryToExtinguishByVal(isSet) && isSet) {
+      // NO-MESSAGE: fire may have been extinguished
+      scream();
+    }
+  }
+}
+
 void negative_reassigned() {
   bool onFire = isBurning();
   if (onFire) {
     onFire = isReallyBurning();
     if (onFire) {
       // NO-MESSAGE: it was a false alarm then
       scream();
     }
if (doSomething(), onFire) {
     }
   }
 }
 
 // ExprWithCleanups doesn't crash
 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_;
   };
 
   bool isSet = false;
   if (RetT::Test(isSet).Ok() && isSet) {
     if (RetT::Test(isSet).Ok() && isSet) {