njames93 added a comment. I feel the refactoring of Aliasing should be in its own PR, with this being a child of it.
================ Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:38 + ifStmt(hasCondition(anyOf( + declRefExpr(hasDeclaration(varDecl().bind("cond_var"))), + binaryOperator(hasOperatorName("&&"), ---------------- Small nit, but using string literals for bound nodes is error prone. may I suggest defining some static strings and using those to prevent any typos. ================ Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:39 + declRefExpr(hasDeclaration(varDecl().bind("cond_var"))), + binaryOperator(hasOperatorName("&&"), + hasEitherOperand(declRefExpr(hasDeclaration( ---------------- Recursive matchers are a pain, but this will miss: ``` if (A && B && ... Y && Z) ``` ================ Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:63 + // Non-local variables may be changed by any call. + if (!CondVar->isLocalVarDeclOrParm()) + return; ---------------- This logic can be moved to the matcher. ``` varDecl(anyOf(parmVarDecl(), hasLocalStorage())) ``` ================ Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:67 + // Volatile variables may be changed by another thread. + if (CondVar->getType().isVolatileQualified()) + return; ---------------- Ditto: ``` varDecl(hasType(isVolatileQualified()))``` ================ Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:71 + // Class instances may be tricky, so restrict ourselves to integers. + if (!CondVar->getType().getTypePtr()->isIntegerType()) + return; ---------------- Ditto, you get the point ================ Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:87-88 + if (isa<DeclRefExpr>(InnerIf->getCond()->IgnoreParenImpCasts()) || + (isa<BinaryOperator>(InnerIf->getCond()) && + cast<BinaryOperator>(InnerIf->getCond())->getOpcode() == BO_LOr)) { + SourceLocation IfBegin = InnerIf->getBeginLoc(); ---------------- use `llvm::dyn_cast`. ================ Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:97 + CharSourceRange::getCharRange(IfBegin, IfEnd)) + << FixItHint::CreateRemoval(CharSourceRange::getCharRange( + Body->getEndLoc(), Body->getEndLoc().getLocWithOffset(1))); ---------------- Again `CharSourceRange::getTokenRange(Body->getEndLoc())` ================ Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:109 + const auto *CondOp = cast<BinaryOperator>(InnerIf->getCond()); + if (isa<DeclRefExpr>(CondOp->getLHS()->IgnoreParenImpCasts()) && + cast<DeclRefExpr>(CondOp->getLHS()->IgnoreParenImpCasts())->getDecl() == ---------------- use `llvm::dyn_cast`. ================ Comment at: clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp:115 + } else { + // For some reason the end location of identifiers is identical to their + // begin location so add their length instead. ---------------- That's because you are getting the character range, if you get the token range it'll get the correct range: `CharSourceRange::getTokenRange` ================ Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:135 `cppcoreguidelines-avoid-goto <cppcoreguidelines-avoid-goto.html>`_, + `cppcoreguidelines-avoid-non-const-global-variables <cppcoreguidelines-avoid-non-const-global-variables.html>`_, `cppcoreguidelines-init-variables <cppcoreguidelines-init-variables.html>`_, "Yes" ---------------- baloghadamsoftware wrote: > What are these changes? I surely did not make them? Maybe the check adder > Python script? Yeah, just undo those unrelated changes, maybe one day the python script will get sorted Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81272/new/ https://reviews.llvm.org/D81272 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits