diff options
8 files changed, 320 insertions, 124 deletions
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp index 9af7979..73d93e3 100644 --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp @@ -381,6 +381,20 @@ static bool usagesReturnRValues(const UsageResult &Usages) { return true; } +/// \brief Returns true if the container is const-qualified. +static bool containerIsConst(const Expr *ContainerExpr, bool Dereference) { + if (const auto *VDec = getReferencedVariable(ContainerExpr)) { + QualType CType = VDec->getType(); + if (Dereference) { + if (!CType->isPointerType()) + return false; + CType = CType->getPointeeType(); + } + return CType.isConstQualified(); + } + return false; +} + LoopConvertCheck::LoopConvertCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), TUInfo(new TUTrackingInfo), MinConfidence(StringSwitch<Confidence::Level>( @@ -401,6 +415,11 @@ void LoopConvertCheck::doConversion( StringRef ContainerString, const UsageResult &Usages, const DeclStmt *AliasDecl, bool AliasUseRequired, bool AliasFromForInit, const ForStmt *TheLoop, RangeDescriptor Descriptor) { + // If there aren't any usages, converting the loop would generate an unused + // variable warning. + if (Usages.size() == 0) + return; + auto Diag = diag(TheLoop->getForLoc(), "use range-based for loop instead"); std::string VarName; @@ -436,11 +455,23 @@ void LoopConvertCheck::doConversion( VarName = Namer.createIndexName(); // First, replace all usages of the array subscript expression with our new // variable. - for (const auto &I : Usages) { - std::string ReplaceText = I.IsArrow ? VarName + "." : VarName; + for (const auto &Usage : Usages) { + std::string ReplaceText; + 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; + } else { + // 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; + } TUInfo->getReplacedVars().insert(std::make_pair(TheLoop, IndexVar)); Diag << FixItHint::CreateReplacement( - CharSourceRange::getTokenRange(I.Range), ReplaceText); + CharSourceRange::getTokenRange(Usage.Range), ReplaceText); } } @@ -537,16 +568,24 @@ void LoopConvertCheck::findAndVerifyUsages( if (FixerKind == LFK_Array) { // The array being indexed by IndexVar was discovered during traversal. ContainerExpr = Finder.getContainerIndexed()->IgnoreParenImpCasts(); + // Very few loops are over expressions that generate arrays rather than // array variables. Consider loops over arrays that aren't just represented // by a variable to be risky conversions. if (!getReferencedVariable(ContainerExpr) && !isDirectMemberExpr(ContainerExpr)) ConfidenceLevel.lowerTo(Confidence::CL_Risky); + + // Use 'const' if the array is const. + if (containerIsConst(ContainerExpr, Descriptor.ContainerNeedsDereference)) + Descriptor.DerefByConstRef = true; + } else if (FixerKind == LFK_PseudoArray) { if (!Descriptor.DerefByValue && !Descriptor.DerefByConstRef) { const UsageResult &Usages = Finder.getUsages(); - if (usagesAreConst(Usages)) { + if (usagesAreConst(Usages) || + containerIsConst(ContainerExpr, + Descriptor.ContainerNeedsDereference)) { Descriptor.DerefByConstRef = true; } else if (usagesReturnRValues(Usages)) { // If the index usages (dereference, subscript, at) return RValues, @@ -558,7 +597,7 @@ void LoopConvertCheck::findAndVerifyUsages( if (!U.Expression || U.Expression->getType().isNull()) continue; QualType Type = U.Expression->getType().getCanonicalType(); - if (U.IsArrow) { + if (U.Kind == Usage::UK_MemberThroughArrow) { if (!Type->isPointerType()) continue; Type = Type->getPointeeType(); @@ -625,6 +664,7 @@ void LoopConvertCheck::check(const MatchFinder::MatchResult &Result) { // expression the loop variable is being tested against instead. const auto *EndCall = Nodes.getStmtAs<CXXMemberCallExpr>(EndCallName); const auto *BoundExpr = Nodes.getStmtAs<Expr>(ConditionBoundName); + // If the loop calls end()/size() after each iteration, lower our confidence // level. if (FixerKind != LFK_Array && !EndVar) diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h index 6773b19..d349fdb 100644 --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h @@ -27,9 +27,9 @@ public: private: struct RangeDescriptor { bool ContainerNeedsDereference; + bool DerefByConstRef; bool DerefByValue; bool IsTriviallyCopyable; - bool DerefByConstRef; }; void doConversion(ASTContext *Context, const VarDecl *IndexVar, diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp index d0b2e81..d12d845 100644 --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp @@ -560,7 +560,7 @@ bool ForLoopIndexUseVisitor::TraverseMemberExpr(MemberExpr *Member) { // If something complicated is happening (i.e. the next token isn't an // arrow), give up on making this work. if (!ArrowLoc.isInvalid()) { - addUsage(Usage(ResultExpr, /*IsArrow=*/true, + addUsage(Usage(ResultExpr, Usage::UK_MemberThroughArrow, SourceRange(Base->getExprLoc(), ArrowLoc))); return true; } @@ -762,7 +762,10 @@ bool ForLoopIndexUseVisitor::TraverseLambdaCapture(LambdaExpr *LE, // FIXME: if the index is captured, it will count as an usage and the // alias (if any) won't work, because it is only used in case of having // exactly one usage. - addUsage(Usage(nullptr, false, C->getLocation())); + addUsage(Usage(nullptr, + C->getCaptureKind() == LCK_ByCopy ? Usage::UK_CaptureByCopy + : Usage::UK_CaptureByRef, + C->getLocation())); } } return VisitorBase::TraverseLambdaCapture(LE, C); diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h index 2fda54e..5290b47 100644 --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h @@ -197,14 +197,39 @@ private: /// \brief The information needed to describe a valid convertible usage /// of an array index or iterator. struct Usage { + enum UsageKind { + // Regular usages of the loop index (the ones not specified below). Some + // examples: + // \code + // int X = 8 * Arr[i]; + // ^~~~~~ + // f(param1, param2, *It); + // ^~~ + // if (Vec[i].SomeBool) {} + // ^~~~~~ + // \endcode + UK_Default, + // Indicates whether this is an access to a member through the arrow + // operator on pointers or iterators. + UK_MemberThroughArrow, + // If the variable is being captured by a lambda, indicates whether the + // capture was done by value or by reference. + UK_CaptureByCopy, + UK_CaptureByRef + }; + // The expression that is going to be converted. Null in case of lambda + // captures. const Expr *Expression; - bool IsArrow; + + UsageKind Kind; + + // Range that covers this usage. SourceRange Range; explicit Usage(const Expr *E) - : Expression(E), IsArrow(false), Range(Expression->getSourceRange()) {} - Usage(const Expr *E, bool IsArrow, SourceRange Range) - : Expression(E), IsArrow(IsArrow), Range(std::move(Range)) {} + : Expression(E), Kind(UK_Default), Range(Expression->getSourceRange()) {} + Usage(const Expr *E, UsageKind Kind, SourceRange Range) + : Expression(E), Kind(Kind), Range(std::move(Range)) {} }; /// \brief A class to encapsulate lowering of the tool's confidence level. diff --git a/clang-tools-extra/test/clang-tidy/Inputs/modernize-loop-convert/structures.h b/clang-tools-extra/test/clang-tidy/Inputs/modernize-loop-convert/structures.h index 24d0eeb..ef31fc0 100644 --- a/clang-tools-extra/test/clang-tidy/Inputs/modernize-loop-convert/structures.h +++ b/clang-tools-extra/test/clang-tidy/Inputs/modernize-loop-convert/structures.h @@ -59,8 +59,9 @@ struct X { }; template<typename ElemType> -class dependent{ +class dependent { public: + dependent<ElemType>(); struct iterator_base { const ElemType& operator*()const; iterator_base& operator ++(); diff --git a/clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp b/clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp index 1979f14..635644a 100644 --- a/clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp +++ b/clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp @@ -7,6 +7,7 @@ namespace Array { const int N = 6; const int NMinusOne = N - 1; int arr[N] = {1, 2, 3, 4, 5, 6}; +const int constArr[N] = {1, 2, 3, 4, 5, 6}; int (*pArr)[N] = &arr; void f() { @@ -17,10 +18,9 @@ void f() { int k; } // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead [modernize-loop-convert] - // CHECK-FIXES: for (auto & elem : arr) { + // CHECK-FIXES: for (auto & elem : arr) // CHECK-FIXES-NEXT: sum += elem; // CHECK-FIXES-NEXT: int k; - // CHECK-FIXES-NEXT: } for (int i = 0; i < N; ++i) { printf("Fibonacci number is %d\n", arr[i]); @@ -53,9 +53,8 @@ void f() { arr[i] += 1; } // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : arr) { + // CHECK-FIXES: for (auto & elem : arr) // CHECK-FIXES-NEXT: elem += 1; - // CHECK-FIXES-NEXT: } for (int i = 0; i < N; ++i) { int x = arr[i] + 2; @@ -77,9 +76,8 @@ void f() { sum += arr[i]; } // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : arr) { + // CHECK-FIXES: for (auto & elem : arr) // CHECK-FIXES-NEXT: sum += elem; - // CHECK-FIXES-NEXT: } for (int i = 0; i < N; ++i) { printf("Fibonacci number %d has address %p\n", arr[i], &arr[i]); @@ -95,9 +93,17 @@ void f() { teas[i].g(); } // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & tea : teas) { + // CHECK-FIXES: for (auto & tea : teas) // CHECK-FIXES-NEXT: tea.g(); - // CHECK-FIXES-NEXT: } +} + +void constArray() { + for (int i = 0; i < N; ++i) { + printf("2 * %d = %d\n", constArr[i], constArr[i] + constArr[i]); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (const auto & elem : constArr) + // CHECK-FIXES-NEXT: printf("2 * %d = %d\n", elem, elem + elem); } struct HasArr { @@ -108,17 +114,15 @@ struct HasArr { printf("%d", Arr[i]); } // CHECK-MESSAGES: :[[@LINE-3]]:5: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : Arr) { + // CHECK-FIXES: for (auto & elem : Arr) // CHECK-FIXES-NEXT: printf("%d", elem); - // CHECK-FIXES-NEXT: } for (int i = 0; i < N; ++i) { printf("%d", ValArr[i].x); } // CHECK-MESSAGES: :[[@LINE-3]]:5: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : ValArr) { + // CHECK-FIXES: for (auto & elem : ValArr) // CHECK-FIXES-NEXT: printf("%d", elem.x); - // CHECK-FIXES-NEXT: } } void explicitThis() { @@ -126,21 +130,19 @@ struct HasArr { printf("%d", this->Arr[i]); } // CHECK-MESSAGES: :[[@LINE-3]]:5: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : this->Arr) { + // CHECK-FIXES: for (auto & elem : this->Arr) // CHECK-FIXES-NEXT: printf("%d", elem); - // CHECK-FIXES-NEXT: } for (int i = 0; i < N; ++i) { printf("%d", this->ValArr[i].x); } // CHECK-MESSAGES: :[[@LINE-3]]:5: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : this->ValArr) { + // CHECK-FIXES: for (auto & elem : this->ValArr) // CHECK-FIXES-NEXT: printf("%d", elem.x); - // CHECK-FIXES-NEXT: } } }; -// Loops whose bounds are value-dependent shold not be converted. +// Loops whose bounds are value-dependent should not be converted. template <int N> void dependentExprBound() { for (int i = 0; i < N; ++i) @@ -263,7 +265,7 @@ void f() { printf("Fibonacci number is %d\n", *it); } // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : v) { + // CHECK-FIXES: for (auto & elem : v) // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", elem); for (dependent<int>::iterator it(v.begin()), e = v.end(); @@ -271,7 +273,7 @@ void f() { printf("Fibonacci number is %d\n", *it); } // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : v) { + // CHECK-FIXES: for (auto & elem : v) // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", elem); doublyDependent<int, int> intmap; @@ -285,13 +287,14 @@ void f() { // PtrSet's iterator dereferences by value so auto & can't be used. { - PtrSet<int *> int_ptrs; - for (PtrSet<int *>::iterator I = int_ptrs.begin(), - E = int_ptrs.end(); + PtrSet<int *> val_int_ptrs; + for (PtrSet<int *>::iterator I = val_int_ptrs.begin(), + E = val_int_ptrs.end(); I != E; ++I) { + (void) *I; } - // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use range-based for loop instead - // CHECK-FIXES: for (auto int_ptr : int_ptrs) { + // CHECK-MESSAGES: :[[@LINE-5]]:5: warning: use range-based for loop instead + // CHECK-FIXES: for (auto val_int_ptr : val_int_ptrs) } // This container uses an iterator where the derefence type is a typedef of @@ -302,9 +305,10 @@ void f() { for (TypedefDerefContainer<int>::iterator I = int_ptrs.begin(), E = int_ptrs.end(); I != E; ++I) { + (void) *I; } - // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & int_ptr : int_ptrs) { + // CHECK-MESSAGES: :[[@LINE-5]]:5: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & int_ptr : int_ptrs) } { @@ -314,10 +318,8 @@ void f() { for (RValueDerefContainer<int>::iterator I = container.begin(), E = container.end(); I != E; ++I) { + (void) *I; } - // CHECK-FIXES: for (RValueDerefContainer<int>::iterator I = container.begin(), - // CHECK-FIXES-NEXT: E = container.end(); - // CHECK-FIXES-NEXT: I != E; ++I) { } } @@ -350,17 +352,11 @@ void different_type() { it != e; ++it) { printf("Fibonacci number is %d\n", *it); } - // CHECK-FIXES: for (dependent<int>::const_iterator it = v.begin(), e = v.end(); - // CHECK-FIXES-NEXT: it != e; ++it) { - // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", *it); for (dependent<int>::const_iterator it(v.begin()), e = v.end(); it != e; ++it) { printf("Fibonacci number is %d\n", *it); } - // CHECK-FIXES: for (dependent<int>::const_iterator it(v.begin()), e = v.end(); - // CHECK-FIXES-NEXT: it != e; ++it) { - // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", *it); } // Tests to ensure that an implicit 'this' is picked up as the container. @@ -380,42 +376,45 @@ public: void doSomething() const; void doLoop() { - for (iterator I = begin(), E = end(); I != E; ++I) { - } + for (iterator I = begin(), E = end(); I != E; ++I) + (void) *I; // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : *this) { + // CHECK-FIXES: for (auto & elem : *this) - for (iterator I = C::begin(), E = C::end(); I != E; ++I) { - } + for (iterator I = C::begin(), E = C::end(); I != E; ++I) + (void) *I; // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : *this) { + // CHECK-FIXES: for (auto & elem : *this) for (iterator I = begin(), E = end(); I != E; ++I) { + (void) *I; doSomething(); } - for (iterator I = begin(); I != end(); ++I) { - } + for (iterator I = begin(); I != end(); ++I) + (void) *I; // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : *this) { + // CHECK-FIXES: for (auto & elem : *this) for (iterator I = begin(); I != end(); ++I) { + (void) *I; doSomething(); } } void doLoop() const { - for (const_iterator I = begin(), E = end(); I != E; ++I) { - } + for (const_iterator I = begin(), E = end(); I != E; ++I) + (void) *I; // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : *this) { + // CHECK-FIXES: for (auto & elem : *this) - for (const_iterator I = C::begin(), E = C::end(); I != E; ++I) { - } + for (const_iterator I = C::begin(), E = C::end(); I != E; ++I) + (void) *I; // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : *this) { + // CHECK-FIXES: for (auto & elem : *this) for (const_iterator I = begin(), E = end(); I != E; ++I) { + (void) *I; doSomething(); } } @@ -431,10 +430,10 @@ public: void doLoop() { // The implicit 'this' will have an Implicit cast to const C2* wrapped // around it. Make sure the replacement still happens. - for (iterator I = begin(), E = end(); I != E; ++I) { - } + for (iterator I = begin(), E = end(); I != E; ++I) + (void) *I; // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : *this) { + // CHECK-FIXES: for (auto & elem : *this) } }; @@ -445,6 +444,8 @@ namespace PseudoArray { const int N = 6; dependent<int> v; dependent<int> *pv; +const dependent<int> constv; +const dependent<int> *pconstv; transparent<dependent<int>> cv; @@ -500,21 +501,62 @@ void f() { // CHECK-FIXES-NEXT: sum += elem + 2; } +void constness() { + int sum = 0; + for (int i = 0, e = constv.size(); i < e; ++i) { + printf("Fibonacci number is %d\n", constv[i]); + sum += constv[i] + 2; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (const auto & elem : constv) + // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", elem); + // CHECK-FIXES-NEXT: sum += elem + 2; + + for (int i = 0, e = constv.size(); i < e; ++i) { + printf("Fibonacci number is %d\n", constv.at(i)); + sum += constv.at(i) + 2; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (const auto & elem : constv) + // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", elem); + // CHECK-FIXES-NEXT: sum += elem + 2; + + for (int i = 0, e = pconstv->size(); i < e; ++i) { + printf("Fibonacci number is %d\n", pconstv->at(i)); + sum += pconstv->at(i) + 2; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (const auto & elem : *pconstv) + // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", elem); + // CHECK-FIXES-NEXT: sum += elem + 2; + + // This test will fail if size() isn't called repeatedly, since it + // returns unsigned int, and 0 is deduced to be signed int. + // FIXME: Insert the necessary explicit conversion, or write out the types + // explicitly. + for (int i = 0; i < pconstv->size(); ++i) { + printf("Fibonacci number is %d\n", (*pconstv).at(i)); + sum += (*pconstv)[i] + 2; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (const auto & elem : *pconstv) + // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", elem); + // CHECK-FIXES-NEXT: sum += elem + 2; +} + // Check for loops that don't mention containers. void noContainer() { for (auto i = 0; i < v.size(); ++i) { } - // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (const auto & elem : v) { for (auto i = 0; i < v.size(); ++i) ; - // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (const auto & elem : v) } struct NoBeginEnd { unsigned size() const; + unsigned& operator[](int); + const unsigned& operator[](int) const; }; struct NoConstBeginEnd { @@ -522,6 +564,8 @@ struct NoConstBeginEnd { unsigned size() const; unsigned* begin(); unsigned* end(); + unsigned& operator[](int); + const unsigned& operator[](int) const; }; struct ConstBeginEnd { @@ -529,30 +573,34 @@ struct ConstBeginEnd { unsigned size() const; unsigned* begin() const; unsigned* end() const; + unsigned& operator[](int); + const unsigned& operator[](int) const; }; // Shouldn't transform pseudo-array uses if the container doesn't provide // begin() and end() of the right const-ness. void NoBeginEndTest() { NoBeginEnd NBE; - for (unsigned i = 0, e = NBE.size(); i < e; ++i) { - } + for (unsigned i = 0, e = NBE.size(); i < e; ++i) + printf("%d\n", NBE[i]); const NoConstBeginEnd const_NCBE; - for (unsigned i = 0, e = const_NCBE.size(); i < e; ++i) { - } + for (unsigned i = 0, e = const_NCBE.size(); i < e; ++i) + printf("%d\n", const_NCBE[i]); ConstBeginEnd CBE; - for (unsigned i = 0, e = CBE.size(); i < e; ++i) { - } + for (unsigned i = 0, e = CBE.size(); i < e; ++i) + printf("%d\n", CBE[i]); // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (const auto & elem : CBE) { + // CHECK-FIXES: for (auto & elem : CBE) + // CHECK-FIXES-NEXT: printf("%d\n", elem); const ConstBeginEnd const_CBE; - for (unsigned i = 0, e = const_CBE.size(); i < e; ++i) { - } + for (unsigned i = 0, e = const_CBE.size(); i < e; ++i) + printf("%d\n", const_CBE[i]); // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (const auto & elem : const_CBE) { + // CHECK-FIXES: for (const auto & elem : const_CBE) + // CHECK-FIXES-NEXT: printf("%d\n", elem); } struct DerefByValue { @@ -570,7 +618,7 @@ void derefByValueTest() { printf("%d\n", DBV[i]); } // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto elem : DBV) { + // CHECK-FIXES: for (auto elem : DBV) // CHECK-FIXES-NEXT: printf("%d\n", elem); for (unsigned i = 0, e = DBV.size(); i < e; ++i) { @@ -578,8 +626,8 @@ void derefByValueTest() { printf("%d\n", DBV[i]); } // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto elem : DBV) { - // CHECK-FIXES-NEXT: auto f = [DBV, elem]() {}; + // CHECK-FIXES: for (auto elem : DBV) + // CHECK-FIXES-NEXT: auto f = [DBV, &elem]() {}; // CHECK-FIXES-NEXT: printf("%d\n", elem); } diff --git a/clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp b/clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp index 365e0f7..9a7575a 100644 --- a/clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp +++ b/clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp @@ -14,10 +14,9 @@ void f() { int b = arr[i][a]; } // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : arr) { + // CHECK-FIXES: for (auto & elem : arr) // CHECK-FIXES-NEXT: int a = 0; // CHECK-FIXES-NEXT: int b = elem[a]; - // CHECK-FIXES-NEXT: } for (int j = 0; j < M; ++j) { int a = 0; @@ -121,7 +120,7 @@ void aliasing() { } // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto alias : IntArr) - // CHECK-FIXES-NEXT: if (alias) { + // CHECK-FIXES-NEXT: if (alias) for (unsigned i = 0; i < N; ++i) { while (int alias = IntArr[i]) { @@ -130,7 +129,7 @@ void aliasing() { } // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto alias : IntArr) - // CHECK-FIXES-NEXT: while (alias) { + // CHECK-FIXES-NEXT: while (alias) for (unsigned i = 0; i < N; ++i) { switch (int alias = IntArr[i]) { @@ -140,7 +139,7 @@ void aliasing() { } // CHECK-MESSAGES: :[[@LINE-6]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto alias : IntArr) - // CHECK-FIXES-NEXT: switch (alias) { + // CHECK-FIXES-NEXT: switch (alias) for (unsigned i = 0; i < N; ++i) { for (int alias = IntArr[i]; alias < N; ++alias) { @@ -149,7 +148,7 @@ void aliasing() { } // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto alias : IntArr) - // CHECK-FIXES-NEXT: for (; alias < N; ++alias) { + // CHECK-FIXES-NEXT: for (; alias < N; ++alias) for (unsigned i = 0; i < N; ++i) { for (unsigned j = 0; int alias = IntArr[i]; ++j) { @@ -158,14 +157,14 @@ void aliasing() { } // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto alias : IntArr) - // CHECK-FIXES-NEXT: for (unsigned j = 0; alias; ++j) { + // CHECK-FIXES-NEXT: for (unsigned j = 0; alias; ++j) struct IntRef { IntRef(const int& i); }; for (int i = 0; i < N; ++i) { IntRef Int(IntArr[i]); } // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : IntArr) { + // CHECK-FIXES: for (auto & elem : IntArr) // CHECK-FIXES-NEXT: IntRef Int(elem); } @@ -288,7 +287,7 @@ void macroConflict() { } // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto & DEFs_it : DEFs) - // CHECK-FIXES-NEXT: if (DEFs_it == DEF) { + // CHECK-FIXES-NEXT: if (DEFs_it == DEF) // CHECK-FIXES-NEXT: printf("I found %d\n", DEFs_it); } @@ -315,8 +314,8 @@ void typeConflict() { T Vals; // Using the name "Val", although it is the name of an existing struct, is // safe in this loop since it will only exist within this scope. - for (T::iterator it = Vals.begin(), e = Vals.end(); it != e; ++it) { - } + for (T::iterator it = Vals.begin(), e = Vals.end(); it != e; ++it) + (void) *it; // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto & Val : Vals) @@ -333,8 +332,8 @@ void typeConflict() { U TDs; // Naming the variable "TD" within this loop is safe because the typedef // was never used within the loop. - for (U::iterator it = TDs.begin(), e = TDs.end(); it != e; ++it) { - } + for (U::iterator it = TDs.begin(), e = TDs.end(); it != e; ++it) + (void) *it; // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto & TD : TDs) @@ -342,8 +341,9 @@ void typeConflict() { for (U::iterator it = TDs.begin(), e = TDs.end(); it != e; ++it) { TD V; V.x = 5; + (void) *it; } - // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto & TDs_it : TDs) // CHECK-FIXES-NEXT: TD V; // CHECK-FIXES-NEXT: V.x = 5; @@ -456,8 +456,8 @@ void f() { } } // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : NestT) { - // CHECK-FIXES-NEXT: for (T::iterator TI = (elem).begin(), TE = (elem).end(); TI != TE; ++TI) { + // CHECK-FIXES: for (auto & elem : NestT) + // CHECK-FIXES-NEXT: for (T::iterator TI = (elem).begin(), TE = (elem).end(); TI != TE; ++TI) // CHECK-FIXES-NEXT: printf("%d", *TI); // The inner loop is also convertible. @@ -468,8 +468,8 @@ void f() { } } // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (const auto & elem : NestS) { - // CHECK-FIXES-NEXT: for (S::const_iterator SI = (elem).begin(), SE = (elem).end(); SI != SE; ++SI) { + // CHECK-FIXES: for (const auto & elem : NestS) + // CHECK-FIXES-NEXT: for (S::const_iterator SI = (elem).begin(), SE = (elem).end(); SI != SE; ++SI) // CHECK-FIXES-NEXT: printf("%d", *SI); for (Nested<S>::const_iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) { @@ -480,7 +480,7 @@ void f() { } } // CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (const auto & s : NestS) { + // CHECK-FIXES: for (const auto & s : NestS) for (Nested<S>::iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) { S &s = *I; @@ -490,7 +490,7 @@ void f() { } } // CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & s : NestS) { + // CHECK-FIXES: for (auto & s : NestS) Foo foo; for (Nested<S>::const_iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) { @@ -501,7 +501,7 @@ void f() { } } // CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (const auto & s : NestS) { + // CHECK-FIXES: for (const auto & s : NestS) for (Nested<S>::iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) { S &s = *I; @@ -511,7 +511,7 @@ void f() { } } // CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & s : NestS) { + // CHECK-FIXES: for (auto & s : NestS) } @@ -623,7 +623,7 @@ void f() { printf("Fibonacci number is %d\n", *it); } // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : v) { + // CHECK-FIXES: for (auto & elem : v) // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", elem); for (dependent<int>::iterator it(v.begin()); @@ -631,7 +631,7 @@ void f() { printf("Fibonacci number is %d\n", *it); } // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : v) { + // CHECK-FIXES: for (auto & elem : v) // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", elem); doublyDependent<int, int> intmap; @@ -684,6 +684,9 @@ void different_type() { namespace Macros { +#define TWO_PARAM(x, y) if (x == y) {} +#define THREE_PARAM(x, y, z) if (x == y) {z;} + const int N = 10; int arr[N]; @@ -692,12 +695,28 @@ void messing_with_macros() { printf("Value: %d\n", arr[i]); } // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : arr) { + // CHECK-FIXES: for (auto & elem : arr) // CHECK-FIXES-NEXT: printf("Value: %d\n", elem); for (int i = 0; i < N; ++i) { printf("Value: %d\n", CONT arr[i]); } + + // FIXME: Right now, clang-tidy does not allow to make insertions in several + // arguments of the same macro call. The following code: + // \code + // for (int i = 0; i < N; ++i) { + // TWO_PARAM(arr[i], arr[i]); + // THREE_PARAM(arr[i], arr[i], arr[i]); + // } + // \endcode + // Should be converted to this: + // \code + // for (auto & elem : arr) { + // TWO_PARAM(elem, elem); + // THREE_PARAM(elem, elem, elem); + // } + // \endcode } } // namespace Macros @@ -708,12 +727,14 @@ template <class Container> void set_union(Container &container) { for (typename Container::const_iterator SI = container.begin(), SE = container.end(); SI != SE; ++SI) { + (void) *SI; } + S s; - for (S::iterator SI = s.begin(), SE = s.end(); SI != SE; ++SI) { - } + for (S::iterator SI = s.begin(), SE = s.end(); SI != SE; ++SI) + (void) *SI; // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : s) { + // CHECK-FIXES: for (auto & elem : s) } void template_instantiation() { @@ -735,7 +756,7 @@ void capturesIndex() { auto F1 = [Arr, I]() { int R1 = Arr[I] + 1; }; // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto & elem : Arr) - // CHECK-FIXES-NEXT: auto F1 = [Arr, elem]() { int R1 = elem + 1; }; + // CHECK-FIXES-NEXT: auto F1 = [Arr, &elem]() { int R1 = elem + 1; }; for (int I = 0; I < N; ++I) auto F2 = [Arr, &I]() { int R2 = Arr[I] + 3; }; @@ -749,8 +770,7 @@ void capturesIndex() { auto F3 = [&Arr, I]() { int R3 = Arr[I]; }; // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto & elem : Arr) - // CHECK-FIXES-NEXT: auto F3 = [&Arr, elem]() { int R3 = elem; }; - // FIXME: this does two copies instead of one. Capture elem by ref? + // CHECK-FIXES-NEXT: auto F3 = [&Arr, &elem]() { int R3 = elem; }; for (int I = 0; I < N; ++I) @@ -764,8 +784,7 @@ void capturesIndex() { auto F5 = [&Arr, I]() { int &R5 = Arr[I]; }; // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto & elem : Arr) - // CHECK-FIXES-NEXT: auto F5 = [&Arr, elem]() { int &R5 = elem; }; - // FIXME: this does one copy instead of none. Capture elem by ref? + // CHECK-FIXES-NEXT: auto F5 = [&Arr, &elem]() { int &R5 = elem; }; for (int I = 0; I < N; ++I) @@ -782,10 +801,9 @@ void capturesIndex() { } // CHECK-MESSAGES: :[[@LINE-6]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto & elem : Arr) - // CHECK-FIXES-NEXT: auto F = [Arr, elem](int k) { + // CHECK-FIXES-NEXT: auto F = [Arr, &elem](int k) // CHECK-FIXES-NEXT: printf("%d\n", elem + k); - // CHECK-FIXES-NEXT: }; - // CHECK-FIXES-NEXT: F(elem); + // CHECK-FIXES: F(elem); } void implicitCapture() { @@ -815,7 +833,7 @@ void implicitCapture() { } // CHECK-MESSAGES: :[[@LINE-6]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto & elem : Arr) - // CHECK-FIXES-NEXT: auto G3 = [&]() { + // CHECK-FIXES-NEXT: auto G3 = [&]() // CHECK-FIXES-NEXT: int R3 = elem; // CHECK-FIXES-NEXT: int J3 = elem + R3; @@ -826,7 +844,7 @@ void implicitCapture() { } // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto & elem : Arr) - // CHECK-FIXES-NEXT: auto G4 = [=]() { + // CHECK-FIXES-NEXT: auto G4 = [=]() // CHECK-FIXES-NEXT: int R4 = elem + 5; // Alias by value. @@ -838,7 +856,7 @@ void implicitCapture() { } // CHECK-MESSAGES: :[[@LINE-6]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto R5 : Arr) - // CHECK-FIXES-NEXT: auto G5 = [&]() { + // CHECK-FIXES-NEXT: auto G5 = [&]() // CHECK-FIXES: int J5 = 8 + R5; // Alias by reference. @@ -850,7 +868,7 @@ void implicitCapture() { } // CHECK-MESSAGES: :[[@LINE-6]]:3: warning: use range-based for loop instead // CHECK-FIXES: for (auto & R6 : Arr) - // CHECK-FIXES-NEXT: auto G6 = [&]() { + // CHECK-FIXES-NEXT: auto G6 = [&]() // CHECK-FIXES: int J6 = -1 + R6; } @@ -883,6 +901,30 @@ void iterators() { auto H5 = [=]() { int R = *I; }; } +void captureByValue() { + // When the index is captured by value, we should replace this by a capture + // by reference. This avoids extra copies. + // FIXME: this could change semantics on array or pseudoarray loops if the + // container is captured by copy. + const int N = 10; + int Arr[N]; + dependent<int> Dep; + + for (int I = 0; I < N; ++I) { + auto C1 = [&Arr, I]() { if (Arr[I] == 1); }; + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : Arr) + // CHECK-FIXES-NEXT: auto C1 = [&Arr, &elem]() { if (elem == 1); }; + + for (unsigned I = 0; I < Dep.size(); ++I) { + auto C2 = [&Dep, I]() { if (Dep[I] == 2); }; + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : Dep) + // CHECK-FIXES-NEXT: auto C2 = [&Dep, &elem]() { if (elem == 2); }; +} + } // namespace Lambdas namespace InitLists { diff --git a/clang-tools-extra/test/clang-tidy/modernize-loop-convert-negative.cpp b/clang-tools-extra/test/clang-tidy/modernize-loop-convert-negative.cpp index bbcd9a2..2d18cbe 100644 --- a/clang-tools-extra/test/clang-tidy/modernize-loop-convert-negative.cpp +++ b/clang-tools-extra/test/clang-tidy/modernize-loop-convert-negative.cpp @@ -290,7 +290,6 @@ const int N = 6; dependent<int> v; dependent<int> *pv; -transparent<dependent<int>> cv; int Sum = 0; // Checks for the Index start and end: @@ -473,3 +472,41 @@ void complexContainer() { } } // namespace NegativeMultiEndCall + +namespace NoUsages { + +const int N = 6; +int arr[N] = {1, 2, 3, 4, 5, 6}; +S s; +dependent<int> v; +int Count = 0; + +void foo(); + +void f() { + for (int I = 0; I < N; ++I) {} + for (int I = 0; I < N; ++I) + printf("Hello world\n"); + for (int I = 0; I < N; ++I) + ++Count; + for (int I = 0; I < N; ++I) + foo(); + + for (S::iterator I = s.begin(), E = s.end(); I != E; ++I) {} + for (S::iterator I = s.begin(), E = s.end(); I != E; ++I) + printf("Hello world\n"); + for (S::iterator I = s.begin(), E = s.end(); I != E; ++I) + ++Count; + for (S::iterator I = s.begin(), E = s.end(); I != E; ++I) + foo(); + + for (int I = 0; I < v.size(); ++I) {} + for (int I = 0; I < v.size(); ++I) + printf("Hello world\n"); + for (int I = 0; I < v.size(); ++I) + ++Count; + for (int I = 0; I < v.size(); ++I) + foo(); +} + +} // namespace NoUsages |