Index: clang-tidy/modernize/LoopConvertCheck.cpp =================================================================== --- clang-tidy/modernize/LoopConvertCheck.cpp +++ clang-tidy/modernize/LoopConvertCheck.cpp @@ -364,6 +364,23 @@ return false; } +/// \brief Returns true when it can be guaranteed that the elements of the +/// container are not being modified. +static bool usagesAreConst(const UsageResult &Usages) { + // FIXME: Make this function more generic. + return Usages.empty(); +} + +/// \brief Returns true if the elements of the container are never accessed +/// by reference. +static bool usagesReturnRValues(const UsageResult &Usages) { + for (const auto &U : Usages) { + if (!U.E->isRValue()) + return false; + } + return true; +} + LoopConvertCheck::LoopConvertCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), TUInfo(new TUTrackingInfo), MinConfidence(StringSwitch( @@ -452,7 +469,8 @@ StringRef MaybeDereference = ContainerNeedsDereference ? "*" : ""; std::string TypeString = AutoRefType.getAsString(); std::string Range = ("(" + TypeString + " " + VarName + " : " + - MaybeDereference + ContainerString + ")").str(); + MaybeDereference + ContainerString + ")") + .str(); Diag << FixItHint::CreateReplacement( CharSourceRange::getTokenRange(ParenRange), Range); TUInfo->getGeneratedDecls().insert(make_pair(TheLoop, VarName)); @@ -464,7 +482,7 @@ StringRef LoopConvertCheck::checkRejections(ASTContext *Context, const Expr *ContainerExpr, const ForStmt *TheLoop) { - // If we already modified the reange of this for loop, don't do any further + // If we already modified the range of this for loop, don't do any further // updates on this iteration. if (TUInfo->getReplacedVars().count(TheLoop)) return ""; @@ -525,6 +543,18 @@ if (!getReferencedVariable(ContainerExpr) && !isDirectMemberExpr(ContainerExpr)) ConfidenceLevel.lowerTo(Confidence::CL_Risky); + } else if (FixerKind == LFK_PseudoArray) { + if (!DerefByValue && !DerefByConstRef) { + const UsageResult &Usages = Finder.getUsages(); + if (usagesAreConst(Usages)) { + // FIXME: check if the type is trivially copiable. + DerefByConstRef = true; + } else if (usagesReturnRValues(Usages)) { + // If the index usages (dereference, subscript, at) return RValues, + // then we should not use a non-const reference. + DerefByValue = true; + } + } } StringRef ContainerString = checkRejections(Context, ContainerExpr, TheLoop); Index: clang-tidy/modernize/LoopConvertUtils.cpp =================================================================== --- clang-tidy/modernize/LoopConvertUtils.cpp +++ clang-tidy/modernize/LoopConvertUtils.cpp @@ -425,12 +425,8 @@ ConfidenceLevel(Confidence::CL_Safe), NextStmtParent(nullptr), CurrStmtParent(nullptr), ReplaceWithAliasUse(false), AliasFromForInit(false) { - if (ContainerExpr) { + if (ContainerExpr) addComponent(ContainerExpr); - FoldingSetNodeID ID; - const Expr *E = ContainerExpr->IgnoreParenImpCasts(); - E->Profile(ID, *Context, true); - } } bool ForLoopIndexUseVisitor::findAndVerifyUsages(const Stmt *Body) { @@ -470,7 +466,7 @@ return true; } - return VisitorBase::TraverseUnaryOperator(Uop); + return VisitorBase::TraverseUnaryDeref(Uop); } /// \brief If the member expression is operator-> (overloaded or not) on @@ -521,7 +517,13 @@ } } - if (Member->isArrow() && Obj && exprReferencesVariable(IndexVar, Obj)) { + if (Obj && exprReferencesVariable(IndexVar, Obj)) { + // Member calls on the iterator with '.' are not allowed. + if (!Member->isArrow()) { + OnlyUsedAsIndex = false; + return true; + } + if (ExprType.isNull()) ExprType = Obj->getType(); @@ -539,7 +541,7 @@ return true; } } - return TraverseStmt(Member->getBase()); + return VisitorBase::TraverseMemberExpr(Member); } /// \brief If a member function call is the at() accessor on the container with @@ -576,7 +578,7 @@ } /// \brief If an overloaded operator call is a dereference of IndexVar or -/// a subscript of a the container with IndexVar as the single argument, +/// a subscript of the container with IndexVar as the single argument, /// include it as a valid usage and prune the traversal. /// /// For example, given @@ -682,9 +684,6 @@ /// i.insert(0); /// for (vector::iterator i = container.begin(), e = container.end(); /// i != e; ++i) -/// i.insert(0); -/// for (vector::iterator i = container.begin(), e = container.end(); -/// i != e; ++i) /// if (i + 1 != e) /// printf("%d", *i); /// \endcode @@ -700,7 +699,8 @@ /// \endcode bool ForLoopIndexUseVisitor::VisitDeclRefExpr(DeclRefExpr *E) { const ValueDecl *TheDecl = E->getDecl(); - if (areSameVariable(IndexVar, TheDecl) || areSameVariable(EndVar, TheDecl)) + if (areSameVariable(IndexVar, TheDecl) || exprReferencesVariable(IndexVar, E) + || areSameVariable(EndVar, TheDecl) || exprReferencesVariable(EndVar, E)) OnlyUsedAsIndex = false; if (containsExpr(Context, &DependentExprs, E)) ConfidenceLevel.lowerTo(Confidence::CL_Risky); Index: test/clang-tidy/modernize-loop-convert-basic.cpp =================================================================== --- test/clang-tidy/modernize-loop-convert-basic.cpp +++ test/clang-tidy/modernize-loop-convert-basic.cpp @@ -427,6 +427,27 @@ } }; +void foo(S::iterator it) {} +class Foo {public: void bar(S::iterator it); }; + +void iterator_used() { + S s; + Foo fo; + + for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) { + foo(it); + } + + for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) { + fo.bar(it); + } + + S::iterator ret; + for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) { + ret = it; + } +} + } // namespace Iterator namespace PseudoArray { @@ -494,12 +515,12 @@ for (auto i = 0; i < v.size(); ++i) { } // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : v) { + // 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 (auto & elem : v) + // CHECK-FIXES: for (const auto & elem : v) } struct NoBeginEnd { @@ -509,15 +530,15 @@ struct NoConstBeginEnd { NoConstBeginEnd(); unsigned size() const; - unsigned begin(); - unsigned end(); + unsigned* begin(); + unsigned* end(); }; struct ConstBeginEnd { ConstBeginEnd(); unsigned size() const; - unsigned begin() const; - unsigned end() const; + unsigned* begin() const; + unsigned* end() const; }; // Shouldn't transform pseudo-array uses if the container doesn't provide @@ -535,13 +556,32 @@ for (unsigned i = 0, e = CBE.size(); i < e; ++i) { } // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : CBE) { + // CHECK-FIXES: for (const auto & elem : CBE) { const ConstBeginEnd const_CBE; for (unsigned i = 0, e = const_CBE.size(); i < e; ++i) { } // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead - // CHECK-FIXES: for (auto & elem : const_CBE) { + // CHECK-FIXES: for (const auto & elem : const_CBE) { +} + +struct DerefByValue { + DerefByValue(); + struct iter { unsigned operator*(); }; + unsigned size() const; + iter begin(); + iter end(); + unsigned operator[](int); +}; + +void DerefByValueTest() { + DerefByValue DBV; + for (unsigned i = 0, e = DBV.size(); i < e; ++i) { + printf("%d\n", DBV[i]); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto && elem : DBV) { + } } // namespace PseudoArray Index: test/clang-tidy/modernize-loop-convert-extra.cpp =================================================================== --- test/clang-tidy/modernize-loop-convert-extra.cpp +++ test/clang-tidy/modernize-loop-convert-extra.cpp @@ -387,6 +387,14 @@ namespace Nesting { +void g(S::iterator it); +void const_g(S::const_iterator it); +class Foo { + public: + void g(S::iterator it); + void const_g(S::const_iterator it); +}; + void f() { const int N = 10; const int M = 15; @@ -454,6 +462,48 @@ // 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::const_iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) { + const S &s = *I; + for (S::const_iterator SI = s.begin(), SE = s.end(); SI != SE; ++SI) { + printf("%d", *SI); + const_g(SI); + } + } + // CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (const auto & s : NestS) { + + for (Nested::iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) { + S &s = *I; + for (S::iterator SI = s.begin(), SE = s.end(); SI != SE; ++SI) { + printf("%d", *SI); + g(SI); + } + } + // CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & s : NestS) { + + Foo foo; + for (Nested::const_iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) { + const S &s = *I; + for (S::const_iterator SI = s.begin(), SE = s.end(); SI != SE; ++SI) { + printf("%d", *SI); + foo.const_g(SI); + } + } + // CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (const auto & s : NestS) { + + for (Nested::iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) { + S &s = *I; + for (S::iterator SI = s.begin(), SE = s.end(); SI != SE; ++SI) { + printf("%d", *SI); + foo.g(SI); + } + } + // CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & s : NestS) { + } } // namespace Nesting