llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: None (flovent) <details> <summary>Changes</summary> Before this patch, this check only handles `VarDecl` as varaibles' declaration in statement, but this will ignore variables in structured bindings (`BindingDecl` in AST), which leads to false positives. Closes #<!-- -->138842. --- Full diff: https://github.com/llvm/llvm-project/pull/144213.diff 5 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp (+31-2) - (modified) clang-tools-extra/clang-tidy/utils/Aliasing.cpp (+7-7) - (modified) clang-tools-extra/clang-tidy/utils/Aliasing.h (+1-1) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp (+81) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp index 07116a7ff15f5..4ceca53e2f7f3 100644 --- a/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp @@ -50,7 +50,7 @@ static Matcher<Stmt> loopEndingStmt(Matcher<Stmt> Internal) { } /// Return whether `Var` was changed in `LoopStmt`. -static bool isChanged(const Stmt *LoopStmt, const VarDecl *Var, +static bool isChanged(const Stmt *LoopStmt, const ValueDecl *Var, ASTContext *Context) { if (const auto *ForLoop = dyn_cast<ForStmt>(LoopStmt)) return (ForLoop->getInc() && @@ -83,6 +83,23 @@ static bool isVarThatIsPossiblyChanged(const Decl *Func, const Stmt *LoopStmt, isChanged(LoopStmt, Var, Context); // FIXME: Track references. } + + if (const auto *BD = dyn_cast<BindingDecl>(DRE->getDecl())) { + if (const auto *DD = + dyn_cast_if_present<DecompositionDecl>(BD->getDecomposedDecl())) { + if (!DD->isLocalVarDeclOrParm()) + return true; + + if (DD->getType().isVolatileQualified()) + return true; + + if (!BD->getType().getTypePtr()->isIntegerType()) + return true; + + return hasPtrOrReferenceInFunc(Func, BD) || + isChanged(LoopStmt, BD, Context); + } + } } else if (isa<MemberExpr, CallExpr, ObjCIvarRefExpr, ObjCPropertyRefExpr, ObjCMessageExpr>(Cond)) { // FIXME: Handle MemberExpr. @@ -124,6 +141,10 @@ static std::string getCondVarNames(const Stmt *Cond) { if (const auto *DRE = dyn_cast<DeclRefExpr>(Cond)) { if (const auto *Var = dyn_cast<VarDecl>(DRE->getDecl())) return std::string(Var->getName()); + + if (const auto *BD = dyn_cast<BindingDecl>(DRE->getDecl())) { + return std::string(BD->getName()); + } } std::string Result; @@ -215,10 +236,18 @@ static bool overlap(ArrayRef<CallGraphNode *> SCC, /// returns true iff `Cond` involves at least one static local variable. static bool hasStaticLocalVariable(const Stmt *Cond) { - if (const auto *DRE = dyn_cast<DeclRefExpr>(Cond)) + if (const auto *DRE = dyn_cast<DeclRefExpr>(Cond)) { if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) if (VD->isStaticLocal()) return true; + + if (const auto *BD = dyn_cast<BindingDecl>(DRE->getDecl())) + if (const auto *DD = + dyn_cast_if_present<DecompositionDecl>(BD->getDecomposedDecl())) + if (DD->isStaticLocal()) + return true; + } + for (const Stmt *Child : Cond->children()) if (Child && hasStaticLocalVariable(Child)) return true; diff --git a/clang-tools-extra/clang-tidy/utils/Aliasing.cpp b/clang-tools-extra/clang-tidy/utils/Aliasing.cpp index 2facf0625605e..cbe4873b5c022 100644 --- a/clang-tools-extra/clang-tidy/utils/Aliasing.cpp +++ b/clang-tools-extra/clang-tidy/utils/Aliasing.cpp @@ -14,14 +14,14 @@ namespace clang::tidy::utils { /// Return whether \p S is a reference to the declaration of \p Var. -static bool isAccessForVar(const Stmt *S, const VarDecl *Var) { +static bool isAccessForVar(const Stmt *S, const ValueDecl *Var) { if (const auto *DRE = dyn_cast<DeclRefExpr>(S)) return DRE->getDecl() == Var; return false; } -static bool capturesByRef(const CXXRecordDecl *RD, const VarDecl *Var) { +static bool capturesByRef(const CXXRecordDecl *RD, const ValueDecl *Var) { return llvm::any_of(RD->captures(), [Var](const LambdaCapture &C) { return C.capturesVariable() && C.getCaptureKind() == LCK_ByRef && C.getCapturedVar() == Var; @@ -29,9 +29,9 @@ static bool capturesByRef(const CXXRecordDecl *RD, const VarDecl *Var) { } /// Return whether \p Var has a pointer or reference in \p S. -static bool isPtrOrReferenceForVar(const Stmt *S, const VarDecl *Var) { +static bool isPtrOrReferenceForVar(const Stmt *S, const ValueDecl *Var) { // Treat block capture by reference as a form of taking a reference. - if (Var->isEscapingByref()) + if (const auto *VD = dyn_cast<VarDecl>(Var); VD && VD->isEscapingByref()) return true; if (const auto *DS = dyn_cast<DeclStmt>(S)) { @@ -61,7 +61,7 @@ static bool isPtrOrReferenceForVar(const Stmt *S, const VarDecl *Var) { } /// Return whether \p Var has a pointer or reference in \p S. -static bool hasPtrOrReferenceInStmt(const Stmt *S, const VarDecl *Var) { +static bool hasPtrOrReferenceInStmt(const Stmt *S, const ValueDecl *Var) { if (isPtrOrReferenceForVar(S, Var)) return true; @@ -77,7 +77,7 @@ static bool hasPtrOrReferenceInStmt(const Stmt *S, const VarDecl *Var) { } static bool refersToEnclosingLambdaCaptureByRef(const Decl *Func, - const VarDecl *Var) { + const ValueDecl *Var) { const auto *MD = dyn_cast<CXXMethodDecl>(Func); if (!MD) return false; @@ -89,7 +89,7 @@ static bool refersToEnclosingLambdaCaptureByRef(const Decl *Func, return capturesByRef(RD, Var); } -bool hasPtrOrReferenceInFunc(const Decl *Func, const VarDecl *Var) { +bool hasPtrOrReferenceInFunc(const Decl *Func, const ValueDecl *Var) { return hasPtrOrReferenceInStmt(Func->getBody(), Var) || refersToEnclosingLambdaCaptureByRef(Func, Var); } diff --git a/clang-tools-extra/clang-tidy/utils/Aliasing.h b/clang-tools-extra/clang-tidy/utils/Aliasing.h index 7dad16fc57f1e..6c0763b766805 100644 --- a/clang-tools-extra/clang-tidy/utils/Aliasing.h +++ b/clang-tools-extra/clang-tidy/utils/Aliasing.h @@ -25,7 +25,7 @@ namespace clang::tidy::utils { /// For `f()` and `n` the function returns ``true`` because `p` is a /// pointer to `n` created in `f()`. -bool hasPtrOrReferenceInFunc(const Decl *Func, const VarDecl *Var); +bool hasPtrOrReferenceInFunc(const Decl *Func, const ValueDecl *Var); } // namespace clang::tidy::utils diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 19ccd1790e757..790ddfaef9103 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -159,6 +159,10 @@ Changes in existing checks false positives on deleted constructors that cannot be used to construct objects, even if they have public or protected access. +- Improved :doc:`bugprone-infinite-loop + <clang-tidy/checks/bugprone/infinite-loop>` check by adding detection for + variables introduced by structured bindings. + - Added an option to :doc:`bugprone-multi-level-implicit-pointer-conversion <clang-tidy/checks/bugprone/multi-level-implicit-pointer-conversion>` to choose whether to enable the check in C code or not. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp index bc14ece3f332c..a8787593da6fe 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop.cpp @@ -711,3 +711,84 @@ void test_local_static_recursion() { while (i >= 0) p(0); // we don't know what p points to so no warning } + +struct PairVal { + int a; + int b; + PairVal(int a, int b) : a(a), b(b) {} +}; + +void structured_binding_infinite_loop1() { + auto [x, y] = PairVal(0, 0); + while (x < 10) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (x) are updated in the loop body [bugprone-infinite-loop] + y++; + } + while (y < 10) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (y) are updated in the loop body [bugprone-infinite-loop] + x++; + } +} + +void structured_binding_infinite_loop2() { + auto [x, y] = PairVal(0, 0); + while (x < 10) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (x) are updated in the loop body [bugprone-infinite-loop] + // No update to x or y + } + while (y < 10) { + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (y) are updated in the loop body [bugprone-infinite-loop] + // No update to x or y + } +} + +void structured_binding_not_infinite1() { + auto [x, y] = PairVal(0, 0); + while (x < 10) { + x++; + } + while (y < 10) { + y++; + } +} + +void volatile_structured_binding_in_condition() { + volatile auto [x, y] = PairVal(0, 0); + while (!x) {} +} + +void test_local_static_structured_binding_recursion() { + static auto [i, _] = PairVal(0, 0); + int j = 0; + + i--; + while (i >= 0) + test_local_static_structured_binding_recursion(); // no warning, recursively decrement i + for (; i >= 0;) + test_local_static_structured_binding_recursion(); // no warning, recursively decrement i + for (; i + j >= 0;) + test_local_static_structured_binding_recursion(); // no warning, recursively decrement i + for (; i >= 0; i--) + ; // no warning, i decrements + while (j >= 0) + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (j) are updated in the loop body [bugprone-infinite-loop] + test_local_static_structured_binding_recursion(); + + int (*p)(int) = 0; + + while (i >= 0) + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop] + p = 0; + while (i >= 0) + p(0); // we don't know what p points to so no warning +} + +struct S { int a; }; +void issue_138842_reduced() { + int x = 10; + auto [y] = S{1}; + + while (y < x) { + y++; + } +} `````````` </details> https://github.com/llvm/llvm-project/pull/144213 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
