AMS21 created this revision.
AMS21 added reviewers: PiotrZSL, njames93, carlosgalvezp.
Herald added subscribers: ChuanqiXu, xazax.hun.
Herald added a project: All.
AMS21 requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Fixes llvm#62951
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D152852
Files:
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-structured-binding.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-structured-binding.cpp
===================================================================
--- /dev/null
+++ 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 <typename T, unsigned long Size>
+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<S, N> 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;
+}
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -320,6 +320,10 @@
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
+ <clang-tidy/checks/modernize/modernize-loop-convert>` generating wrong code
+ when using structured bindings.
+
- In :doc:`modernize-use-default-member-init
<clang-tidy/checks/modernize/use-default-member-init>` count template
constructors toward hand written constructors so that they are skipped if more
Index: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -8,6 +8,7 @@
#include "LoopConvertCheck.h"
#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Basic/LLVM.h"
#include "clang/Basic/LangOptions.h"
@@ -24,6 +25,9 @@
#include <optional>
#include <utility>
+// TODO: Remove me
+#include <iostream>
+
using namespace clang::ast_matchers;
using namespace llvm;
@@ -533,32 +537,50 @@
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<FixItHint> FixIts;
if (VarNameFromAlias) {
const auto *AliasVar = cast<VarDecl>(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 (isa<DecompositionDecl>(AliasDecl->getSingleDecl())) {
+ const auto *AliasDecompositionDecl =
+ cast<DecompositionDecl>(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 +599,7 @@
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 +608,9 @@
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<ParenExpr>()) {
@@ -611,8 +634,9 @@
// 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 +678,7 @@
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 +698,8 @@
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 +713,7 @@
} 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<CXXOperatorCallExpr>(ContainerExpr))
+ if (const auto *E = dyn_cast<CXXOperatorCallExpr>(ContainerExpr))
if (E->getOperator() != OO_Subscript)
ContainerExpr = E->getArg(0);
ContainerString =
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
