Index: clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.h =================================================================== --- clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.h +++ clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.h @@ -25,22 +25,27 @@ void check(const ast_matchers::MatchFinder::MatchResult &Result) override; private: + struct RangeDescriptor { + bool ContainerNeedsDereference; + bool DerefByValue; + bool IsTriviallyCopyable; + bool DerefByConstRef; + }; + void doConversion(ASTContext *Context, const VarDecl *IndexVar, const VarDecl *MaybeContainer, StringRef ContainerString, const UsageResult &Usages, const DeclStmt *AliasDecl, bool AliasUseRequired, bool AliasFromForInit, - const ForStmt *TheLoop, bool ContainerNeedsDereference, - bool DerefByValue, bool DerefByConstRef); + const ForStmt *TheLoop, RangeDescriptor Descriptor); StringRef checkRejections(ASTContext *Context, const Expr *ContainerExpr, const ForStmt *TheLoop); void findAndVerifyUsages(ASTContext *Context, const VarDecl *LoopVar, const VarDecl *EndVar, const Expr *ContainerExpr, - const Expr *BoundExpr, - bool ContainerNeedsDereference, bool DerefByValue, - bool DerefByConstRef, const ForStmt *TheLoop, - LoopFixerKind FixerKind); + const Expr *BoundExpr, const ForStmt *TheLoop, + LoopFixerKind FixerKind, RangeDescriptor Descriptor); + std::unique_ptr TUInfo; Confidence::Level MinConfidence; }; Index: clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.cpp +++ clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.cpp @@ -375,7 +375,7 @@ /// by reference. static bool usagesReturnRValues(const UsageResult &Usages) { for (const auto &U : Usages) { - if (!U.Expression->isRValue()) + if (U.Expression && !U.Expression->isRValue()) return false; } return true; @@ -400,8 +400,7 @@ ASTContext *Context, const VarDecl *IndexVar, const VarDecl *MaybeContainer, StringRef ContainerString, const UsageResult &Usages, const DeclStmt *AliasDecl, bool AliasUseRequired, bool AliasFromForInit, - const ForStmt *TheLoop, bool ContainerNeedsDereference, bool DerefByValue, - bool DerefByConstRef) { + const ForStmt *TheLoop, RangeDescriptor Descriptor) { auto Diag = diag(TheLoop->getForLoc(), "use range-based for loop instead"); std::string VarName; @@ -457,16 +456,17 @@ // If an iterator's operator*() returns a 'T&' we can bind that to 'auto&'. // If operator*() returns 'T' we can bind that to 'auto&&' which will deduce // to 'T&&&'. - if (DerefByValue) { - AutoRefType = Context->getRValueReferenceType(AutoRefType); + if (Descriptor.DerefByValue) { + if (!Descriptor.IsTriviallyCopyable) + AutoRefType = Context->getRValueReferenceType(AutoRefType); } else { - if (DerefByConstRef) + if (Descriptor.DerefByConstRef) AutoRefType = Context->getConstType(AutoRefType); AutoRefType = Context->getLValueReferenceType(AutoRefType); } } - StringRef MaybeDereference = ContainerNeedsDereference ? "*" : ""; + StringRef MaybeDereference = Descriptor.ContainerNeedsDereference ? "*" : ""; std::string TypeString = AutoRefType.getAsString(); std::string Range = ("(" + TypeString + " " + VarName + " : " + MaybeDereference + ContainerString + ")") @@ -518,11 +518,11 @@ /// of the index variable and convert the loop if possible. void LoopConvertCheck::findAndVerifyUsages( ASTContext *Context, const VarDecl *LoopVar, const VarDecl *EndVar, - const Expr *ContainerExpr, const Expr *BoundExpr, - bool ContainerNeedsDereference, bool DerefByValue, bool DerefByConstRef, - const ForStmt *TheLoop, LoopFixerKind FixerKind) { + const Expr *ContainerExpr, const Expr *BoundExpr, const ForStmt *TheLoop, + LoopFixerKind FixerKind, RangeDescriptor Descriptor) { ForLoopIndexUseVisitor Finder(Context, LoopVar, EndVar, ContainerExpr, - BoundExpr, ContainerNeedsDereference); + BoundExpr, + Descriptor.ContainerNeedsDereference); if (ContainerExpr) { ComponentFinderASTVisitor ComponentFinder; @@ -544,15 +544,28 @@ !isDirectMemberExpr(ContainerExpr)) ConfidenceLevel.lowerTo(Confidence::CL_Risky); } else if (FixerKind == LFK_PseudoArray) { - if (!DerefByValue && !DerefByConstRef) { + if (!Descriptor.DerefByValue && !Descriptor.DerefByConstRef) { const UsageResult &Usages = Finder.getUsages(); if (usagesAreConst(Usages)) { - // FIXME: check if the type is trivially copiable. - DerefByConstRef = true; + Descriptor.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; + Descriptor.DerefByValue = true; + // Try to find the type of the elements on the container from the + // usages. + for (const Usage &U : Usages) { + if (!U.Expression || U.Expression->getType().isNull()) + continue; + QualType Type = U.Expression->getType().getCanonicalType(); + if (U.IsArrow) { + if (!Type->isPointerType()) + continue; + Type = Type->getPointeeType(); + } + Descriptor.IsTriviallyCopyable = + Type.isTriviallyCopyableType(*Context); + } } } } @@ -565,7 +578,7 @@ doConversion(Context, LoopVar, getReferencedVariable(ContainerExpr), ContainerString, Finder.getUsages(), Finder.getAliasDecl(), Finder.aliasUseRequired(), Finder.aliasFromForInit(), TheLoop, - ContainerNeedsDereference, DerefByValue, DerefByConstRef); + Descriptor); } void LoopConvertCheck::registerMatchers(MatchFinder *Finder) { @@ -618,15 +631,13 @@ ConfidenceLevel.lowerTo(Confidence::CL_Reasonable); const Expr *ContainerExpr = nullptr; - bool DerefByValue = false; - bool DerefByConstRef = false; - bool ContainerNeedsDereference = false; + RangeDescriptor Descriptor{false, false, false, false}; // FIXME: Try to put most of this logic inside a matcher. Currently, matchers // don't allow the ight-recursive checks in digThroughConstructors. if (FixerKind == LFK_Iterator) { ContainerExpr = findContainer(Context, LoopVar->getInit(), EndVar ? EndVar->getInit() : EndCall, - &ContainerNeedsDereference); + &Descriptor.ContainerNeedsDereference); QualType InitVarType = InitVar->getType(); QualType CanonicalInitVarType = InitVarType.getCanonicalType(); @@ -643,6 +654,8 @@ // un-qualified pointee types match otherwise we don't use auto. if (!Context->hasSameUnqualifiedType(InitPointeeType, BeginPointeeType)) return; + Descriptor.IsTriviallyCopyable = + BeginPointeeType.isTriviallyCopyableType(*Context); } else { // Check for qualified types to avoid conversions from non-const to const // iterator types. @@ -650,17 +663,19 @@ return; } - DerefByValue = Nodes.getNodeAs(DerefByValueResultName) != nullptr; - if (!DerefByValue) { + const auto *DerefByValueType = + Nodes.getNodeAs(DerefByValueResultName); + Descriptor.DerefByValue = DerefByValueType; + if (!Descriptor.DerefByValue) { if (const auto *DerefType = Nodes.getNodeAs(DerefByRefResultName)) { // A node will only be bound with DerefByRefResultName if we're dealing // with a user-defined iterator type. Test the const qualification of // the reference type. - DerefByConstRef = (*DerefType) - ->getAs() - ->getPointeeType() - .isConstQualified(); + Descriptor.DerefByConstRef = (*DerefType) + ->getAs() + ->getPointeeType() + .isConstQualified(); } else { // By nature of the matcher this case is triggered only for built-in // iterator types (i.e. pointers). @@ -671,12 +686,14 @@ // If the initializer and variable have both the same type just use auto // otherwise we test for const qualification of the pointed-at type. if (!Context->hasSameType(InitPointeeType, BeginPointeeType)) - DerefByConstRef = InitPointeeType.isConstQualified(); + Descriptor.DerefByConstRef = InitPointeeType.isConstQualified(); } } else { // If the dereference operator returns by value then test for the // canonical const qualification of the init variable type. - DerefByConstRef = CanonicalInitVarType.isConstQualified(); + Descriptor.DerefByConstRef = CanonicalInitVarType.isConstQualified(); + Descriptor.IsTriviallyCopyable = + DerefByValueType->isTriviallyCopyableType(*Context); } } else if (FixerKind == LFK_PseudoArray) { if (!EndCall) @@ -685,7 +702,7 @@ const auto *Member = dyn_cast(EndCall->getCallee()); if (!Member) return; - ContainerNeedsDereference = Member->isArrow(); + Descriptor.ContainerNeedsDereference = Member->isArrow(); } // We must know the container or an array length bound. @@ -696,8 +713,7 @@ return; findAndVerifyUsages(Context, LoopVar, EndVar, ContainerExpr, BoundExpr, - ContainerNeedsDereference, DerefByValue, DerefByConstRef, - TheLoop, FixerKind); + TheLoop, FixerKind, Descriptor); } } // namespace modernize Index: clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertUtils.cpp =================================================================== --- clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertUtils.cpp +++ clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertUtils.cpp @@ -349,11 +349,13 @@ // Check that the declared type is the same as (or a reference to) the // container type. + QualType InitType = Init->getType(); QualType DeclarationType = VDecl->getType(); - if (DeclarationType->isReferenceType()) + if (!DeclarationType.isNull() && DeclarationType->isReferenceType()) DeclarationType = DeclarationType.getNonReferenceType(); - QualType InitType = Init->getType(); - if (!Context->hasSameUnqualifiedType(DeclarationType, InitType)) + + if (InitType.isNull() || DeclarationType.isNull() || + !Context->hasSameUnqualifiedType(DeclarationType, InitType)) return false; switch (Init->getStmtClass()) { Index: clang-tools-extra/trunk/test/clang-tidy/modernize-loop-convert-basic.cpp =================================================================== --- clang-tools-extra/trunk/test/clang-tidy/modernize-loop-convert-basic.cpp +++ clang-tools-extra/trunk/test/clang-tidy/modernize-loop-convert-basic.cpp @@ -291,7 +291,7 @@ I != E; ++I) { } // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use range-based for loop instead - // CHECK-FIXES: for (auto && int_ptr : int_ptrs) { + // CHECK-FIXES: for (auto int_ptr : int_ptrs) { } // This container uses an iterator where the derefence type is a typedef of @@ -564,14 +564,23 @@ unsigned operator[](int); }; -void DerefByValueTest() { +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) { + // CHECK-FIXES: for (auto elem : DBV) { + // CHECK-FIXES-NEXT: printf("%d\n", elem); + for (unsigned i = 0, e = DBV.size(); i < e; ++i) { + auto f = [DBV, i]() {}; + 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-NEXT: printf("%d\n", elem); } } // namespace PseudoArray