[clang-tools-extra] dc4359f - [clang-tidy] Fix wrong code generation for `modernize-loop-convert` with structured bindings.
Piotr Zegar via cfe-commits
cfe-commits at lists.llvm.org
Wed Jun 14 11:58:07 PDT 2023
Author: AMS21
Date: 2023-06-14T18:57:29Z
New Revision: dc4359fecf02f4230b4a5b900077c29d1b90f4ab
URL: https://github.com/llvm/llvm-project/commit/dc4359fecf02f4230b4a5b900077c29d1b90f4ab
DIFF: https://github.com/llvm/llvm-project/commit/dc4359fecf02f4230b4a5b900077c29d1b90f4ab.diff
LOG: [clang-tidy] Fix wrong code generation for `modernize-loop-convert` with structured bindings.
Fixes llvm#62951
Reviewed By: PiotrZSL
Differential Revision: https://reviews.llvm.org/D152852
Added:
clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-structured-binding.cpp
Modified:
clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
Removed:
################################################################################
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<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 (const auto *AliasDecompositionDecl =
+ dyn_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 +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<ParenExpr>()) {
@@ -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<CXXOperatorCallExpr>(ContainerExpr))
+ if (const auto *E = dyn_cast<CXXOperatorCallExpr>(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
+ <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
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 <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;
+}
More information about the cfe-commits
mailing list