diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp index 87504e93ca240..618d1860ff8bd 100644 --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp @@ -533,32 +533,48 @@ void LoopConvertCheck::doConversion( const ValueDecl *MaybeContainer, const UsageResult &Usages, const DeclStmt *AliasDecl, bool AliasUseRequired, bool AliasFromForInit, const ForStmt *Loop, RangeDescriptor Descriptor) { - std::string VarName; + std::string VarNameOrStructuredBinding; bool VarNameFromAlias = (Usages.size() == 1) && AliasDecl; bool AliasVarIsRef = false; bool CanCopy = true; std::vector FixIts; if (VarNameFromAlias) { const auto *AliasVar = cast(AliasDecl->getSingleDecl()); - VarName = AliasVar->getName().str(); - - // Use the type of the alias if it's not the same - QualType AliasVarType = AliasVar->getType(); - assert(!AliasVarType.isNull() && "Type in VarDecl is null"); - if (AliasVarType->isReferenceType()) { - AliasVarType = AliasVarType.getNonReferenceType(); - AliasVarIsRef = true; + + // Handle structured bindings + if (const auto *AliasDecompositionDecl = + dyn_cast(AliasDecl->getSingleDecl())) { + VarNameOrStructuredBinding = "["; + + assert(!AliasDecompositionDecl->bindings().empty() && "No bindings"); + for (const BindingDecl *Binding : AliasDecompositionDecl->bindings()) { + VarNameOrStructuredBinding += Binding->getName().str() + ", "; + } + + VarNameOrStructuredBinding.erase(VarNameOrStructuredBinding.size() - 2, + 2); + VarNameOrStructuredBinding += "]"; + } else { + VarNameOrStructuredBinding = AliasVar->getName().str(); + + // Use the type of the alias if it's not the same + QualType AliasVarType = AliasVar->getType(); + assert(!AliasVarType.isNull() && "Type in VarDecl is null"); + if (AliasVarType->isReferenceType()) { + AliasVarType = AliasVarType.getNonReferenceType(); + AliasVarIsRef = true; + } + if (Descriptor.ElemType.isNull() || + !Context->hasSameUnqualifiedType(AliasVarType, Descriptor.ElemType)) + Descriptor.ElemType = AliasVarType; } - if (Descriptor.ElemType.isNull() || - !Context->hasSameUnqualifiedType(AliasVarType, Descriptor.ElemType)) - Descriptor.ElemType = AliasVarType; // We keep along the entire DeclStmt to keep the correct range here. SourceRange ReplaceRange = AliasDecl->getSourceRange(); std::string ReplacementText; if (AliasUseRequired) { - ReplacementText = VarName; + ReplacementText = VarNameOrStructuredBinding; } else if (AliasFromForInit) { // FIXME: Clang includes the location of the ';' but only for DeclStmt's // in a for loop's init clause. Need to put this ';' back while removing @@ -577,7 +593,7 @@ void LoopConvertCheck::doConversion( VariableNamer Namer(&TUInfo->getGeneratedDecls(), &TUInfo->getParentFinder().getStmtToParentStmtMap(), Loop, IndexVar, MaybeContainer, Context, NamingStyle); - VarName = Namer.createIndexName(); + VarNameOrStructuredBinding = Namer.createIndexName(); // First, replace all usages of the array subscript expression with our new // variable. for (const auto &Usage : Usages) { @@ -586,8 +602,9 @@ void LoopConvertCheck::doConversion( if (Usage.Expression) { // If this is an access to a member through the arrow operator, after // the replacement it must be accessed through the '.' operator. - ReplaceText = Usage.Kind == Usage::UK_MemberThroughArrow ? VarName + "." - : VarName; + ReplaceText = Usage.Kind == Usage::UK_MemberThroughArrow + ? VarNameOrStructuredBinding + "." + : VarNameOrStructuredBinding; auto Parents = Context->getParents(*Usage.Expression); if (Parents.size() == 1) { if (const auto *Paren = Parents[0].get()) { @@ -611,8 +628,9 @@ void LoopConvertCheck::doConversion( // The Usage expression is only null in case of lambda captures (which // are VarDecl). If the index is captured by value, add '&' to capture // by reference instead. - ReplaceText = - Usage.Kind == Usage::UK_CaptureByCopy ? "&" + VarName : VarName; + ReplaceText = Usage.Kind == Usage::UK_CaptureByCopy + ? "&" + VarNameOrStructuredBinding + : VarNameOrStructuredBinding; } TUInfo->getReplacedVars().insert(std::make_pair(Loop, IndexVar)); FixIts.push_back(FixItHint::CreateReplacement( @@ -654,7 +672,7 @@ void LoopConvertCheck::doConversion( llvm::raw_svector_ostream Output(Range); Output << '('; Type.print(Output, getLangOpts()); - Output << ' ' << VarName << " : "; + Output << ' ' << VarNameOrStructuredBinding << " : "; if (Descriptor.NeedsReverseCall) Output << getReverseFunction() << '('; if (Descriptor.ContainerNeedsDereference) @@ -674,7 +692,8 @@ void LoopConvertCheck::doConversion( FixIts.push_back(*Insertion); } diag(Loop->getForLoc(), "use range-based for loop instead") << FixIts; - TUInfo->getGeneratedDecls().insert(make_pair(Loop, VarName)); + TUInfo->getGeneratedDecls().insert( + make_pair(Loop, VarNameOrStructuredBinding)); } /// Returns a string which refers to the container iterated over. @@ -688,7 +707,7 @@ StringRef LoopConvertCheck::getContainerString(ASTContext *Context, } else { // For CXXOperatorCallExpr such as vector_ptr->size() we want the class // object vector_ptr, but for vector[2] we need the whole expression. - if (const auto* E = dyn_cast(ContainerExpr)) + if (const auto *E = dyn_cast(ContainerExpr)) if (E->getOperator() != OO_Subscript) ContainerExpr = E->getArg(0); ContainerString = diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index aadda7efac690..6e69b6019c548 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -345,6 +345,10 @@ Changes in existing checks using macro between namespace declarations, to fix false positive when using namespace with attributes and to support nested inline namespace introduced in c++20. +- Fixed an issue in `modernize-loop-convert + ` generating wrong code + when using structured bindings. + - In :doc:`modernize-use-default-member-init ` count template constructors toward hand written constructors so that they are skipped if more diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-structured-binding.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-structured-binding.cpp new file mode 100644 index 0000000000000..dab58a3a5c307 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-structured-binding.cpp @@ -0,0 +1,49 @@ +// RUN: %check_clang_tidy -std=c++17-or-later %s modernize-loop-convert %t + +struct S { + int a{0}; + int b{1}; +}; + +template +struct array { + + T *begin() { return data; } + const T* cbegin() const { return data; } + T *end() { return data + Size; } + const T *cend() const { return data + Size; } + + T data[Size]; +}; + +const int N = 6; +S Arr[N]; + +void f() { + int Sum = 0; + + for (int I = 0; I < N; ++I) { + auto [a, b] = Arr[I]; + Sum += a + b; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead [modernize-loop-convert] + // CHECK-FIXES: for (auto [a, b] : Arr) { + // CHECK-FIXES-NEXT: Sum += a + b; + + array arr; + for (auto* It = arr.begin(); It != arr.end(); ++It) { + auto [a, b] = *It; + Sum = a + b; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead [modernize-loop-convert] + // CHECK-FIXES: for (auto [a, b] : arr) { + // CHECK-FIXES-NEXT: Sum = a + b; + + for (auto* It = arr.cbegin(); It != arr.cend(); ++It) { + auto [a, b] = *It; + Sum = a + b; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead [modernize-loop-convert] + // CHECK-FIXES: for (auto [a, b] : arr) { + // CHECK-FIXES-NEXT: Sum = a + b; +}