diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt index 45cdd440c9e2..d2bb2c106565 100644 --- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt @@ -1,6 +1,8 @@ set(LLVM_LINK_COMPONENTS support) add_clang_library(clangTidyModernizeModule + LoopConvertCheck.cpp + LoopConvertUtils.cpp ModernizeTidyModule.cpp PassByValueCheck.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp new file mode 100644 index 000000000000..fc9a0ff34ae4 --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp @@ -0,0 +1,668 @@ +//===--- LoopConvertCheck.cpp - clang-tidy---------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "LoopConvertCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang; +using namespace clang::ast_matchers; +using namespace llvm; + +namespace clang { +namespace tidy { +namespace modernize { + +const char LoopNameArray[] = "forLoopArray"; +const char LoopNameIterator[] = "forLoopIterator"; +const char LoopNamePseudoArray[] = "forLoopPseudoArray"; +const char ConditionBoundName[] = "conditionBound"; +const char ConditionVarName[] = "conditionVar"; +const char IncrementVarName[] = "incrementVar"; +const char InitVarName[] = "initVar"; +const char BeginCallName[] = "beginCall"; +const char EndCallName[] = "endCall"; +const char ConditionEndVarName[] = "conditionEndVar"; +const char EndVarName[] = "endVar"; +const char DerefByValueResultName[] = "derefByValueResult"; +const char DerefByRefResultName[] = "derefByRefResult"; + +// shared matchers +static const TypeMatcher AnyType = anything(); + +static const StatementMatcher IntegerComparisonMatcher = + expr(ignoringParenImpCasts( + declRefExpr(to(varDecl(hasType(isInteger())).bind(ConditionVarName))))); + +static const DeclarationMatcher InitToZeroMatcher = + varDecl(hasInitializer(ignoringParenImpCasts(integerLiteral(equals(0))))) + .bind(InitVarName); + +static const StatementMatcher IncrementVarMatcher = + declRefExpr(to(varDecl(hasType(isInteger())).bind(IncrementVarName))); + +/// \brief The matcher for loops over arrays. +/// +/// In this general example, assuming 'j' and 'k' are of integral type: +/// \code +/// for (int i = 0; j < 3 + 2; ++k) { ... } +/// \endcode +/// The following string identifiers are bound to these parts of the AST: +/// ConditionVarName: 'j' (as a VarDecl) +/// ConditionBoundName: '3 + 2' (as an Expr) +/// InitVarName: 'i' (as a VarDecl) +/// IncrementVarName: 'k' (as a VarDecl) +/// LoopName: The entire for loop (as a ForStmt) +/// +/// Client code will need to make sure that: +/// - The three index variables identified by the matcher are the same +/// VarDecl. +/// - The index variable is only used as an array index. +/// - All arrays indexed by the loop are the same. +StatementMatcher makeArrayLoopMatcher() { + StatementMatcher ArrayBoundMatcher = + expr(hasType(isInteger())).bind(ConditionBoundName); + + return forStmt( + hasLoopInit(declStmt(hasSingleDecl(InitToZeroMatcher))), + hasCondition(anyOf( + binaryOperator(hasOperatorName("<"), + hasLHS(IntegerComparisonMatcher), + hasRHS(ArrayBoundMatcher)), + binaryOperator(hasOperatorName(">"), hasLHS(ArrayBoundMatcher), + hasRHS(IntegerComparisonMatcher)))), + hasIncrement(unaryOperator(hasOperatorName("++"), + hasUnaryOperand(IncrementVarMatcher)))) + .bind(LoopNameArray); +} + +/// \brief The matcher used for iterator-based for loops. +/// +/// This matcher is more flexible than array-based loops. It will match +/// catch loops of the following textual forms (regardless of whether the +/// iterator type is actually a pointer type or a class type): +/// +/// Assuming f, g, and h are of type containerType::iterator, +/// \code +/// for (containerType::iterator it = container.begin(), +/// e = createIterator(); f != g; ++h) { ... } +/// for (containerType::iterator it = container.begin(); +/// f != anotherContainer.end(); ++h) { ... } +/// \endcode +/// The following string identifiers are bound to the parts of the AST: +/// InitVarName: 'it' (as a VarDecl) +/// ConditionVarName: 'f' (as a VarDecl) +/// LoopName: The entire for loop (as a ForStmt) +/// In the first example only: +/// EndVarName: 'e' (as a VarDecl) +/// ConditionEndVarName: 'g' (as a VarDecl) +/// In the second example only: +/// EndCallName: 'container.end()' (as a CXXMemberCallExpr) +/// +/// Client code will need to make sure that: +/// - The iterator variables 'it', 'f', and 'h' are the same. +/// - The two containers on which 'begin' and 'end' are called are the same. +/// - If the end iterator variable 'g' is defined, it is the same as 'f'. +StatementMatcher makeIteratorLoopMatcher() { + StatementMatcher BeginCallMatcher = + memberCallExpr(argumentCountIs(0), callee(methodDecl(hasName("begin")))) + .bind(BeginCallName); + + DeclarationMatcher InitDeclMatcher = + varDecl(hasInitializer(anyOf(ignoringParenImpCasts(BeginCallMatcher), + materializeTemporaryExpr( + ignoringParenImpCasts(BeginCallMatcher)), + hasDescendant(BeginCallMatcher)))) + .bind(InitVarName); + + DeclarationMatcher EndDeclMatcher = + varDecl(hasInitializer(anything())).bind(EndVarName); + + StatementMatcher EndCallMatcher = + memberCallExpr(argumentCountIs(0), callee(methodDecl(hasName("end")))); + + StatementMatcher IteratorBoundMatcher = + expr(anyOf(ignoringParenImpCasts( + declRefExpr(to(varDecl().bind(ConditionEndVarName)))), + ignoringParenImpCasts(expr(EndCallMatcher).bind(EndCallName)), + materializeTemporaryExpr(ignoringParenImpCasts( + expr(EndCallMatcher).bind(EndCallName))))); + + StatementMatcher IteratorComparisonMatcher = expr( + ignoringParenImpCasts(declRefExpr(to(varDecl().bind(ConditionVarName))))); + + StatementMatcher OverloadedNEQMatcher = + operatorCallExpr(hasOverloadedOperatorName("!="), argumentCountIs(2), + hasArgument(0, IteratorComparisonMatcher), + hasArgument(1, IteratorBoundMatcher)); + + // This matcher tests that a declaration is a CXXRecordDecl that has an + // overloaded operator*(). If the operator*() returns by value instead of by + // reference then the return type is tagged with DerefByValueResultName. + internal::Matcher TestDerefReturnsByValue = + hasType(recordDecl(hasMethod(allOf( + hasOverloadedOperatorName("*"), + anyOf( + // Tag the return type if it's by value. + returns(qualType(unless(hasCanonicalType(referenceType()))) + .bind(DerefByValueResultName)), + returns( + // Skip loops where the iterator's operator* returns an + // rvalue reference. This is just weird. + qualType(unless(hasCanonicalType(rValueReferenceType()))) + .bind(DerefByRefResultName))))))); + + return forStmt( + hasLoopInit(anyOf(declStmt(declCountIs(2), + containsDeclaration(0, InitDeclMatcher), + containsDeclaration(1, EndDeclMatcher)), + declStmt(hasSingleDecl(InitDeclMatcher)))), + hasCondition( + anyOf(binaryOperator(hasOperatorName("!="), + hasLHS(IteratorComparisonMatcher), + hasRHS(IteratorBoundMatcher)), + binaryOperator(hasOperatorName("!="), + hasLHS(IteratorBoundMatcher), + hasRHS(IteratorComparisonMatcher)), + OverloadedNEQMatcher)), + hasIncrement(anyOf( + unaryOperator(hasOperatorName("++"), + hasUnaryOperand(declRefExpr( + to(varDecl(hasType(pointsTo(AnyType))) + .bind(IncrementVarName))))), + operatorCallExpr( + hasOverloadedOperatorName("++"), + hasArgument( + 0, declRefExpr(to(varDecl(TestDerefReturnsByValue) + .bind(IncrementVarName)))))))) + .bind(LoopNameIterator); +} + +/// \brief The matcher used for array-like containers (pseudoarrays). +/// +/// This matcher is more flexible than array-based loops. It will match +/// loops of the following textual forms (regardless of whether the +/// iterator type is actually a pointer type or a class type): +/// +/// Assuming f, g, and h are of type containerType::iterator, +/// \code +/// for (int i = 0, j = container.size(); f < g; ++h) { ... } +/// for (int i = 0; f < container.size(); ++h) { ... } +/// \endcode +/// The following string identifiers are bound to the parts of the AST: +/// InitVarName: 'i' (as a VarDecl) +/// ConditionVarName: 'f' (as a VarDecl) +/// LoopName: The entire for loop (as a ForStmt) +/// In the first example only: +/// EndVarName: 'j' (as a VarDecl) +/// ConditionEndVarName: 'g' (as a VarDecl) +/// In the second example only: +/// EndCallName: 'container.size()' (as a CXXMemberCallExpr) +/// +/// Client code will need to make sure that: +/// - The index variables 'i', 'f', and 'h' are the same. +/// - The containers on which 'size()' is called is the container indexed. +/// - The index variable is only used in overloaded operator[] or +/// container.at(). +/// - If the end iterator variable 'g' is defined, it is the same as 'j'. +/// - The container's iterators would not be invalidated during the loop. +StatementMatcher makePseudoArrayLoopMatcher() { + // Test that the incoming type has a record declaration that has methods + // called 'begin' and 'end'. If the incoming type is const, then make sure + // these methods are also marked const. + // + // FIXME: To be completely thorough this matcher should also ensure the + // return type of begin/end is an iterator that dereferences to the same as + // what operator[] or at() returns. Such a test isn't likely to fail except + // for pathological cases. + // + // FIXME: Also, a record doesn't necessarily need begin() and end(). Free + // functions called begin() and end() taking the container as an argument + // are also allowed. + TypeMatcher RecordWithBeginEnd = qualType( + anyOf(qualType(isConstQualified(), + hasDeclaration(recordDecl( + hasMethod(methodDecl(hasName("begin"), isConst())), + hasMethod(methodDecl(hasName("end"), + isConst())))) // hasDeclaration + ), // qualType + qualType(unless(isConstQualified()), + hasDeclaration( + recordDecl(hasMethod(hasName("begin")), + hasMethod(hasName("end"))))) // qualType + )); + + StatementMatcher SizeCallMatcher = memberCallExpr( + argumentCountIs(0), + callee(methodDecl(anyOf(hasName("size"), hasName("length")))), + on(anyOf(hasType(pointsTo(RecordWithBeginEnd)), + hasType(RecordWithBeginEnd)))); + + StatementMatcher EndInitMatcher = + expr(anyOf(ignoringParenImpCasts(expr(SizeCallMatcher).bind(EndCallName)), + explicitCastExpr(hasSourceExpression(ignoringParenImpCasts( + expr(SizeCallMatcher).bind(EndCallName)))))); + + DeclarationMatcher EndDeclMatcher = + varDecl(hasInitializer(EndInitMatcher)).bind(EndVarName); + + StatementMatcher IndexBoundMatcher = + expr(anyOf(ignoringParenImpCasts(declRefExpr(to( + varDecl(hasType(isInteger())).bind(ConditionEndVarName)))), + EndInitMatcher)); + + return forStmt( + hasLoopInit( + anyOf(declStmt(declCountIs(2), + containsDeclaration(0, InitToZeroMatcher), + containsDeclaration(1, EndDeclMatcher)), + declStmt(hasSingleDecl(InitToZeroMatcher)))), + hasCondition(anyOf( + binaryOperator(hasOperatorName("<"), + hasLHS(IntegerComparisonMatcher), + hasRHS(IndexBoundMatcher)), + binaryOperator(hasOperatorName(">"), hasLHS(IndexBoundMatcher), + hasRHS(IntegerComparisonMatcher)))), + hasIncrement(unaryOperator(hasOperatorName("++"), + hasUnaryOperand(IncrementVarMatcher)))) + .bind(LoopNamePseudoArray); +} + +/// \brief Determine whether Init appears to be an initializing an iterator. +/// +/// If it is, returns the object whose begin() or end() method is called, and +/// the output parameter isArrow is set to indicate whether the initialization +/// is called via . or ->. +static const Expr *getContainerFromBeginEndCall(const Expr *Init, bool IsBegin, + bool *IsArrow) { + // FIXME: Maybe allow declaration/initialization outside of the for loop. + const auto *TheCall = + dyn_cast_or_null(digThroughConstructors(Init)); + if (!TheCall || TheCall->getNumArgs() != 0) + return nullptr; + + const auto *Member = dyn_cast(TheCall->getCallee()); + if (!Member) + return nullptr; + StringRef Name = Member->getMemberDecl()->getName(); + StringRef TargetName = IsBegin ? "begin" : "end"; + if (Name != TargetName) + return nullptr; + + const Expr *SourceExpr = Member->getBase(); + if (!SourceExpr) + return nullptr; + + *IsArrow = Member->isArrow(); + return SourceExpr; +} + +/// \brief Determines the container whose begin() and end() functions are called +/// for an iterator-based loop. +/// +/// BeginExpr must be a member call to a function named "begin()", and EndExpr +/// must be a member. +static const Expr *findContainer(ASTContext *Context, const Expr *BeginExpr, + const Expr *EndExpr, + bool *ContainerNeedsDereference) { + // Now that we know the loop variable and test expression, make sure they are + // valid. + bool BeginIsArrow = false; + bool EndIsArrow = false; + const Expr *BeginContainerExpr = + getContainerFromBeginEndCall(BeginExpr, /*IsBegin=*/true, &BeginIsArrow); + if (!BeginContainerExpr) + return nullptr; + + const Expr *EndContainerExpr = + getContainerFromBeginEndCall(EndExpr, /*IsBegin=*/false, &EndIsArrow); + // Disallow loops that try evil things like this (note the dot and arrow): + // for (IteratorType It = Obj.begin(), E = Obj->end(); It != E; ++It) { } + if (!EndContainerExpr || BeginIsArrow != EndIsArrow || + !areSameExpr(Context, EndContainerExpr, BeginContainerExpr)) + return nullptr; + + *ContainerNeedsDereference = BeginIsArrow; + return BeginContainerExpr; +} + +/// \brief Obtain the original source code text from a SourceRange. +static StringRef getStringFromRange(SourceManager &SourceMgr, + const LangOptions &LangOpts, + SourceRange Range) { + if (SourceMgr.getFileID(Range.getBegin()) != + SourceMgr.getFileID(Range.getEnd())) + return nullptr; + + return Lexer::getSourceText(CharSourceRange(Range, true), SourceMgr, + LangOpts); +} + +/// \brief If the given expression is actually a DeclRefExpr, find and return +/// the underlying VarDecl; otherwise, return NULL. +static const VarDecl *getReferencedVariable(const Expr *E) { + if (const DeclRefExpr *DRE = getDeclRef(E)) + return dyn_cast(DRE->getDecl()); + return nullptr; +} + +/// \brief Returns true when the given expression is a member expression +/// whose base is `this` (implicitly or not). +static bool isDirectMemberExpr(const Expr *E) { + if (const auto *Member = dyn_cast(E->IgnoreParenImpCasts())) + return isa(Member->getBase()->IgnoreParenImpCasts()); + return false; +} + +LoopConvertCheck::LoopConvertCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), TUInfo(new TUTrackingInfo), + MinConfidence(StringSwitch( + Options.get("MinConfidence", "reasonable")) + .Case("safe", Confidence::CL_Safe) + .Case("risky", Confidence::CL_Risky) + .Default(Confidence::CL_Reasonable)) {} + +void LoopConvertCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + SmallVector Confs{"risky", "reasonable", "safe"}; + Options.store(Opts, "MinConfidence", Confs[static_cast(MinConfidence)]); +} + +/// \brief Computes the changes needed to convert a given for loop, and +/// applies it. +void LoopConvertCheck::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) { + auto Diag = diag(TheLoop->getForLoc(), "use range-based for loop instead"); + + std::string VarName; + bool VarNameFromAlias = (Usages.size() == 1) && AliasDecl; + bool AliasVarIsRef = false; + + if (VarNameFromAlias) { + const auto *AliasVar = cast(AliasDecl->getSingleDecl()); + VarName = AliasVar->getName().str(); + AliasVarIsRef = AliasVar->getType()->isReferenceType(); + + // We keep along the entire DeclStmt to keep the correct range here. + const SourceRange &ReplaceRange = AliasDecl->getSourceRange(); + + std::string ReplacementText; + if (AliasUseRequired) { + ReplacementText = VarName; + } 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 + // the declaration of the alias variable. This is probably a bug. + ReplacementText = ";"; + } + + Diag << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(ReplaceRange), ReplacementText); + // No further replacements are made to the loop, since the iterator or index + // was used exactly once - in the initialization of AliasVar. + } else { + VariableNamer Namer(&TUInfo->getGeneratedDecls(), + &TUInfo->getParentFinder().getStmtToParentStmtMap(), + TheLoop, IndexVar, MaybeContainer, Context); + VarName = Namer.createIndexName(); + // First, replace all usages of the array subscript expression with our new + // variable. + for (const auto &I : Usages) { + StringRef ReplaceText = I.IsArrow ? VarName + "." : VarName; + TUInfo->getReplacedVars().insert(std::make_pair(TheLoop, IndexVar)); + Diag << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(I.Range), ReplaceText); + } + } + + // Now, we need to construct the new range expression. + SourceRange ParenRange(TheLoop->getLParenLoc(), TheLoop->getRParenLoc()); + + QualType AutoRefType = Context->getAutoDeductType(); + + // If the new variable name is from the aliased variable, then the reference + // type for the new variable should only be used if the aliased variable was + // declared as a reference. + if (!VarNameFromAlias || AliasVarIsRef) { + // 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); + } else { + if (DerefByConstRef) + AutoRefType = Context->getConstType(AutoRefType); + AutoRefType = Context->getLValueReferenceType(AutoRefType); + } + } + + StringRef MaybeDereference = ContainerNeedsDereference ? "*" : ""; + StringRef TypeString = AutoRefType.getAsString(); + StringRef Range = ("(" + TypeString + " " + VarName + " : " + + MaybeDereference + ContainerString + ")") + .str(); + + Diag << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(ParenRange), Range); + TUInfo->getGeneratedDecls().insert(make_pair(TheLoop, VarName)); +} + +/// \brief Determine if the change should be deferred or rejected, returning +/// text which refers to the container iterated over if the change should +/// proceed. +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 + // updates on this iteration. + if (TUInfo->getReplacedVars().count(TheLoop)) + return ""; + + Context->getTranslationUnitDecl(); + TUInfo->getParentFinder(); + TUInfo->getParentFinder().gatherAncestors(Context->getTranslationUnitDecl()); + // Ensure that we do not try to move an expression dependent on a local + // variable declared inside the loop outside of it. + DependencyFinderASTVisitor DependencyFinder( + &TUInfo->getParentFinder().getStmtToParentStmtMap(), + &TUInfo->getParentFinder().getDeclToParentStmtMap(), + &TUInfo->getReplacedVars(), TheLoop); + + // FIXME: Determine when the external dependency isn't an expression converted + // by another loop. + if (DependencyFinder.dependsOnInsideVariable(ContainerExpr)) + return ""; + + StringRef ContainerString; + if (isa(ContainerExpr->IgnoreParenImpCasts())) { + ContainerString = "this"; + } else { + ContainerString = + getStringFromRange(Context->getSourceManager(), Context->getLangOpts(), + ContainerExpr->getSourceRange()); + } + + return ContainerString; +} + +/// \brief Given a loop header that would be convertible, discover all usages +/// 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) { + ForLoopIndexUseVisitor Finder(Context, LoopVar, EndVar, ContainerExpr, + BoundExpr, ContainerNeedsDereference); + + if (ContainerExpr) { + ComponentFinderASTVisitor ComponentFinder; + ComponentFinder.findExprComponents(ContainerExpr->IgnoreParenImpCasts()); + Finder.addComponents(ComponentFinder.getComponents()); + } + + if (!Finder.findAndVerifyUsages(TheLoop->getBody())) + return; + + Confidence ConfidenceLevel(Finder.getConfidenceLevel()); + 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); + } + + StringRef ContainerString = checkRejections(Context, ContainerExpr, TheLoop); + + if (ContainerString.empty() || ConfidenceLevel.getLevel() < MinConfidence) + return; + + doConversion(Context, LoopVar, getReferencedVariable(ContainerExpr), + ContainerString, Finder.getUsages(), Finder.getAliasDecl(), + Finder.aliasUseRequired(), Finder.aliasFromForInit(), TheLoop, + ContainerNeedsDereference, DerefByValue, DerefByConstRef); +} + +void LoopConvertCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(makeArrayLoopMatcher(), this); + Finder->addMatcher(makeIteratorLoopMatcher(), this); + Finder->addMatcher(makePseudoArrayLoopMatcher(), this); +} + +void LoopConvertCheck::check(const MatchFinder::MatchResult &Result) { + const BoundNodes &Nodes = Result.Nodes; + Confidence ConfidenceLevel(Confidence::CL_Safe); + ASTContext *Context = Result.Context; + + const ForStmt *TheLoop; + LoopFixerKind FixerKind; + + if ((TheLoop = Nodes.getStmtAs(LoopNameArray))) { + FixerKind = LFK_Array; + } else if ((TheLoop = Nodes.getStmtAs(LoopNameIterator))) { + FixerKind = LFK_Iterator; + } else { + TheLoop = Nodes.getStmtAs(LoopNamePseudoArray); + assert(TheLoop && "Bad Callback. No for statement"); + FixerKind = LFK_PseudoArray; + } + + // Check that we have exactly one index variable and at most one end variable. + const auto *LoopVar = Nodes.getDeclAs(IncrementVarName); + const auto *CondVar = Nodes.getDeclAs(ConditionVarName); + const auto *InitVar = Nodes.getDeclAs(InitVarName); + if (!areSameVariable(LoopVar, CondVar) || !areSameVariable(LoopVar, InitVar)) + return; + const auto *EndVar = Nodes.getDeclAs(EndVarName); + const auto *ConditionEndVar = Nodes.getDeclAs(ConditionEndVarName); + if (EndVar && !areSameVariable(EndVar, ConditionEndVar)) + return; + + // If the end comparison isn't a variable, we can try to work with the + // expression the loop variable is being tested against instead. + const auto *EndCall = Nodes.getStmtAs(EndCallName); + const auto *BoundExpr = Nodes.getStmtAs(ConditionBoundName); + // If the loop calls end()/size() after each iteration, lower our confidence + // level. + if (FixerKind != LFK_Array && !EndVar) + ConfidenceLevel.lowerTo(Confidence::CL_Reasonable); + + const Expr *ContainerExpr = nullptr; + bool DerefByValue = false; + bool DerefByConstRef = false; + bool ContainerNeedsDereference = 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); + + QualType InitVarType = InitVar->getType(); + QualType CanonicalInitVarType = InitVarType.getCanonicalType(); + + const auto *BeginCall = Nodes.getNodeAs(BeginCallName); + assert(BeginCall && "Bad Callback. No begin call expression"); + QualType CanonicalBeginType = + BeginCall->getMethodDecl()->getReturnType().getCanonicalType(); + if (CanonicalBeginType->isPointerType() && + CanonicalInitVarType->isPointerType()) { + QualType BeginPointeeType = CanonicalBeginType->getPointeeType(); + QualType InitPointeeType = CanonicalInitVarType->getPointeeType(); + // If the initializer and the variable are both pointers check if the + // un-qualified pointee types match otherwise we don't use auto. + if (!Context->hasSameUnqualifiedType(InitPointeeType, BeginPointeeType)) + return; + } else { + // Check for qualified types to avoid conversions from non-const to const + // iterator types. + if (!Context->hasSameType(CanonicalInitVarType, CanonicalBeginType)) + return; + } + + DerefByValue = Nodes.getNodeAs(DerefByValueResultName) != nullptr; + if (!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(); + } else { + // By nature of the matcher this case is triggered only for built-in + // iterator types (i.e. pointers). + assert(isa(CanonicalInitVarType) && + "Non-class iterator type is not a pointer type"); + QualType InitPointeeType = CanonicalInitVarType->getPointeeType(); + QualType BeginPointeeType = CanonicalBeginType->getPointeeType(); + // 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(); + } + } else { + // If the dereference operator returns by value then test for the + // canonical const qualification of the init variable type. + DerefByConstRef = CanonicalInitVarType.isConstQualified(); + } + } else if (FixerKind == LFK_PseudoArray) { + if (!EndCall) + return; + ContainerExpr = EndCall->getImplicitObjectArgument(); + const auto *Member = dyn_cast(EndCall->getCallee()); + if (!Member) + return; + ContainerNeedsDereference = Member->isArrow(); + } + + // We must know the container or an array length bound. + if (!ContainerExpr && !BoundExpr) + return; + + if (ConfidenceLevel.getLevel() < MinConfidence) + return; + + findAndVerifyUsages(Context, LoopVar, EndVar, ContainerExpr, BoundExpr, + ContainerNeedsDereference, DerefByValue, DerefByConstRef, + TheLoop, FixerKind); +} + +} // namespace modernize +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h new file mode 100644 index 000000000000..b7590638d772 --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h @@ -0,0 +1,52 @@ +//===--- LoopConvertCheck.h - clang-tidy-------------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_LOOP_CONVERT_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_LOOP_CONVERT_H + +#include "../ClangTidy.h" +#include "LoopConvertUtils.h" + +namespace clang { +namespace tidy { +namespace modernize { + +class LoopConvertCheck : public ClangTidyCheck { +public: + LoopConvertCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + 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); + + 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); + std::unique_ptr TUInfo; + Confidence::Level MinConfidence; +}; + +} // namespace modernize +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_LOOP_CONVERT_H diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp new file mode 100644 index 000000000000..ac8210f54c98 --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp @@ -0,0 +1,817 @@ +//===--- LoopConvertUtils.cpp - clang-tidy --------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "LoopConvertUtils.h" + +using namespace clang::ast_matchers; +using namespace clang::tooling; +using namespace clang; +using namespace llvm; + +namespace clang { +namespace tidy { +namespace modernize { + +/// \brief Tracks a stack of parent statements during traversal. +/// +/// All this really does is inject push_back() before running +/// RecursiveASTVisitor::TraverseStmt() and pop_back() afterwards. The Stmt atop +/// the stack is the parent of the current statement (NULL for the topmost +/// statement). +bool StmtAncestorASTVisitor::TraverseStmt(Stmt *Statement) { + StmtAncestors.insert(std::make_pair(Statement, StmtStack.back())); + StmtStack.push_back(Statement); + RecursiveASTVisitor::TraverseStmt(Statement); + StmtStack.pop_back(); + return true; +} + +/// \brief Keep track of the DeclStmt associated with each VarDecl. +/// +/// Combined with StmtAncestors, this provides roughly the same information as +/// Scope, as we can map a VarDecl to its DeclStmt, then walk up the parent tree +/// using StmtAncestors. +bool StmtAncestorASTVisitor::VisitDeclStmt(DeclStmt *Decls) { + for (const auto *decl : Decls->decls()) { + if (const auto *V = dyn_cast(decl)) + DeclParents.insert(std::make_pair(V, Decls)); + } + return true; +} + +/// \brief record the DeclRefExpr as part of the parent expression. +bool ComponentFinderASTVisitor::VisitDeclRefExpr(DeclRefExpr *E) { + Components.push_back(E); + return true; +} + +/// \brief record the MemberExpr as part of the parent expression. +bool ComponentFinderASTVisitor::VisitMemberExpr(MemberExpr *Member) { + Components.push_back(Member); + return true; +} + +/// \brief Forward any DeclRefExprs to a check on the referenced variable +/// declaration. +bool DependencyFinderASTVisitor::VisitDeclRefExpr(DeclRefExpr *DeclRef) { + if (auto *V = dyn_cast_or_null(DeclRef->getDecl())) + return VisitVarDecl(V); + return true; +} + +/// \brief Determine if any this variable is declared inside the ContainingStmt. +bool DependencyFinderASTVisitor::VisitVarDecl(VarDecl *V) { + const Stmt *Curr = DeclParents->lookup(V); + // First, see if the variable was declared within an inner scope of the loop. + while (Curr != nullptr) { + if (Curr == ContainingStmt) { + DependsOnInsideVariable = true; + return false; + } + Curr = StmtParents->lookup(Curr); + } + + // Next, check if the variable was removed from existence by an earlier + // iteration. + for (const auto &I : *ReplacedVars) { + if (I.second == V) { + DependsOnInsideVariable = true; + return false; + } + } + return true; +} + +/// \brief If we already created a variable for TheLoop, check to make sure +/// that the name was not already taken. +bool DeclFinderASTVisitor::VisitForStmt(ForStmt *TheLoop) { + StmtGeneratedVarNameMap::const_iterator I = GeneratedDecls->find(TheLoop); + if (I != GeneratedDecls->end() && I->second == Name) { + Found = true; + return false; + } + return true; +} + +/// \brief If any named declaration within the AST subtree has the same name, +/// then consider Name already taken. +bool DeclFinderASTVisitor::VisitNamedDecl(NamedDecl *D) { + const IdentifierInfo *Ident = D->getIdentifier(); + if (Ident && Ident->getName() == Name) { + Found = true; + return false; + } + return true; +} + +/// \brief Forward any declaration references to the actual check on the +/// referenced declaration. +bool DeclFinderASTVisitor::VisitDeclRefExpr(DeclRefExpr *DeclRef) { + if (auto *D = dyn_cast(DeclRef->getDecl())) + return VisitNamedDecl(D); + return true; +} + +/// \brief If the new variable name conflicts with any type used in the loop, +/// then we mark that variable name as taken. +bool DeclFinderASTVisitor::VisitTypeLoc(TypeLoc TL) { + QualType QType = TL.getType(); + + // Check if our name conflicts with a type, to handle for typedefs. + if (QType.getAsString() == Name) { + Found = true; + return false; + } + // Check for base type conflicts. For example, when a struct is being + // referenced in the body of the loop, the above getAsString() will return the + // whole type (ex. "struct s"), but will be caught here. + if (const IdentifierInfo *Ident = QType.getBaseTypeIdentifier()) { + if (Ident->getName() == Name) { + Found = true; + return false; + } + } + return true; +} + +/// \brief Look through conversion/copy constructors to find the explicit +/// initialization expression, returning it is found. +/// +/// The main idea is that given +/// vector v; +/// we consider either of these initializations +/// vector::iterator it = v.begin(); +/// vector::iterator it(v.begin()); +/// and retrieve `v.begin()` as the expression used to initialize `it` but do +/// not include +/// vector::iterator it; +/// vector::iterator it(v.begin(), 0); // if this constructor existed +/// as being initialized from `v.begin()` +const Expr *digThroughConstructors(const Expr *E) { + if (!E) + return nullptr; + E = E->IgnoreParenImpCasts(); + if (const auto *ConstructExpr = dyn_cast(E)) { + // The initial constructor must take exactly one parameter, but base class + // and deferred constructors can take more. + if (ConstructExpr->getNumArgs() != 1 || + ConstructExpr->getConstructionKind() != CXXConstructExpr::CK_Complete) + return nullptr; + E = ConstructExpr->getArg(0); + if (const auto *Temp = dyn_cast(E)) + E = Temp->GetTemporaryExpr(); + return digThroughConstructors(E); + } + return E; +} + +/// \brief Returns true when two Exprs are equivalent. +bool areSameExpr(ASTContext *Context, const Expr *First, const Expr *Second) { + if (!First || !Second) + return false; + + llvm::FoldingSetNodeID FirstID, SecondID; + First->Profile(FirstID, *Context, true); + Second->Profile(SecondID, *Context, true); + return FirstID == SecondID; +} + +/// \brief Returns the DeclRefExpr represented by E, or NULL if there isn't one. +const DeclRefExpr *getDeclRef(const Expr *E) { + return dyn_cast(E->IgnoreParenImpCasts()); +} + +/// \brief Returns true when two ValueDecls are the same variable. +bool areSameVariable(const ValueDecl *First, const ValueDecl *Second) { + return First && Second && + First->getCanonicalDecl() == Second->getCanonicalDecl(); +} + +/// \brief Determines if an expression is a declaration reference to a +/// particular variable. +static bool exprReferencesVariable(const ValueDecl *Target, const Expr *E) { + if (!Target || !E) + return false; + const DeclRefExpr *Decl = getDeclRef(E); + return Decl && areSameVariable(Target, Decl->getDecl()); +} + +/// \brief If the expression is a dereference or call to operator*(), return the +/// operand. Otherwise, return NULL. +static const Expr *getDereferenceOperand(const Expr *E) { + if (const auto *Uop = dyn_cast(E)) + return Uop->getOpcode() == UO_Deref ? Uop->getSubExpr() : nullptr; + + if (const auto *OpCall = dyn_cast(E)) { + return OpCall->getOperator() == OO_Star && OpCall->getNumArgs() == 1 + ? OpCall->getArg(0) + : nullptr; + } + + return nullptr; +} + +/// \brief Returns true when the Container contains an Expr equivalent to E. +template +static bool containsExpr(ASTContext *Context, const ContainerT *Container, + const Expr *E) { + llvm::FoldingSetNodeID ID; + E->Profile(ID, *Context, true); + for (const auto &I : *Container) { + if (ID == I.second) + return true; + } + return false; +} + +/// \brief Returns true when the index expression is a declaration reference to +/// IndexVar. +/// +/// If the index variable is `index`, this function returns true on +/// arrayExpression[index]; +/// containerExpression[index]; +/// but not +/// containerExpression[notIndex]; +static bool isIndexInSubscriptExpr(const Expr *IndexExpr, + const VarDecl *IndexVar) { + const DeclRefExpr *Idx = getDeclRef(IndexExpr); + return Idx && Idx->getType()->isIntegerType() && + areSameVariable(IndexVar, Idx->getDecl()); +} + +/// \brief Returns true when the index expression is a declaration reference to +/// IndexVar, Obj is the same expression as SourceExpr after all parens and +/// implicit casts are stripped off. +/// +/// If PermitDeref is true, IndexExpression may +/// be a dereference (overloaded or builtin operator*). +/// +/// This function is intended for array-like containers, as it makes sure that +/// both the container and the index match. +/// If the loop has index variable `index` and iterates over `container`, then +/// isIndexInSubscriptExpr returns true for +/// \code +/// container[index] +/// container.at(index) +/// container->at(index) +/// \endcode +/// but not for +/// \code +/// container[notIndex] +/// notContainer[index] +/// \endcode +/// If PermitDeref is true, then isIndexInSubscriptExpr additionally returns +/// true on these expressions: +/// \code +/// (*container)[index] +/// (*container).at(index) +/// \endcode +static bool isIndexInSubscriptExpr(ASTContext *Context, const Expr *IndexExpr, + const VarDecl *IndexVar, const Expr *Obj, + const Expr *SourceExpr, bool PermitDeref) { + if (!SourceExpr || !Obj || !isIndexInSubscriptExpr(IndexExpr, IndexVar)) + return false; + + if (areSameExpr(Context, SourceExpr->IgnoreParenImpCasts(), + Obj->IgnoreParenImpCasts())) + return true; + + if (const Expr *InnerObj = getDereferenceOperand(Obj->IgnoreParenImpCasts())) + if (PermitDeref && areSameExpr(Context, SourceExpr->IgnoreParenImpCasts(), + InnerObj->IgnoreParenImpCasts())) + return true; + + return false; +} + +/// \brief Returns true when Opcall is a call a one-parameter dereference of +/// IndexVar. +/// +/// For example, if the index variable is `index`, returns true for +/// *index +/// but not +/// index +/// *notIndex +static bool isDereferenceOfOpCall(const CXXOperatorCallExpr *OpCall, + const VarDecl *IndexVar) { + return OpCall->getOperator() == OO_Star && OpCall->getNumArgs() == 1 && + exprReferencesVariable(IndexVar, OpCall->getArg(0)); +} + +/// \brief Returns true when Uop is a dereference of IndexVar. +/// +/// For example, if the index variable is `index`, returns true for +/// *index +/// but not +/// index +/// *notIndex +static bool isDereferenceOfUop(const UnaryOperator *Uop, + const VarDecl *IndexVar) { + return Uop->getOpcode() == UO_Deref && + exprReferencesVariable(IndexVar, Uop->getSubExpr()); +} + +/// \brief Determines whether the given Decl defines a variable initialized to +/// the loop object. +/// +/// This is intended to find cases such as +/// \code +/// for (int i = 0; i < arraySize(arr); ++i) { +/// T t = arr[i]; +/// // use t, do not use i +/// } +/// \endcode +/// and +/// \code +/// for (iterator i = container.begin(), e = container.end(); i != e; ++i) { +/// T t = *i; +/// // use t, do not use i +/// } +/// \endcode +static bool isAliasDecl(const Decl *TheDecl, const VarDecl *IndexVar) { + const auto *VDecl = dyn_cast(TheDecl); + if (!VDecl) + return false; + if (!VDecl->hasInit()) + return false; + + const Expr *Init = + digThroughConstructors(VDecl->getInit()->IgnoreParenImpCasts()); + if (!Init) + return false; + + switch (Init->getStmtClass()) { + case Stmt::ArraySubscriptExprClass: { + const auto *E = cast(Init); + // We don't really care which array is used here. We check to make sure + // it was the correct one later, since the AST will traverse it next. + return isIndexInSubscriptExpr(E->getIdx(), IndexVar); + } + + case Stmt::UnaryOperatorClass: + return isDereferenceOfUop(cast(Init), IndexVar); + + case Stmt::CXXOperatorCallExprClass: { + const auto *OpCall = cast(Init); + if (OpCall->getOperator() == OO_Star) + return isDereferenceOfOpCall(OpCall, IndexVar); + if (OpCall->getOperator() == OO_Subscript) { + assert(OpCall->getNumArgs() == 2); + return true; + } + break; + } + + case Stmt::CXXMemberCallExprClass: + return true; + + default: + break; + } + return false; +} + +/// \brief Determines whether the bound of a for loop condition expression is +/// the same as the statically computable size of ArrayType. +/// +/// Given +/// \code +/// const int N = 5; +/// int arr[N]; +/// \endcode +/// This is intended to permit +/// \code +/// for (int i = 0; i < N; ++i) { /* use arr[i] */ } +/// for (int i = 0; i < arraysize(arr); ++i) { /* use arr[i] */ } +/// \endcode +static bool arrayMatchesBoundExpr(ASTContext *Context, + const QualType &ArrayType, + const Expr *ConditionExpr) { + if (!ConditionExpr || ConditionExpr->isValueDependent()) + return false; + const ConstantArrayType *ConstType = + Context->getAsConstantArrayType(ArrayType); + if (!ConstType) + return false; + llvm::APSInt ConditionSize; + if (!ConditionExpr->isIntegerConstantExpr(ConditionSize, *Context)) + return false; + llvm::APSInt ArraySize(ConstType->getSize()); + return llvm::APSInt::isSameValue(ConditionSize, ArraySize); +} + +ForLoopIndexUseVisitor::ForLoopIndexUseVisitor(ASTContext *Context, + const VarDecl *IndexVar, + const VarDecl *EndVar, + const Expr *ContainerExpr, + const Expr *ArrayBoundExpr, + bool ContainerNeedsDereference) + : Context(Context), IndexVar(IndexVar), EndVar(EndVar), + ContainerExpr(ContainerExpr), ArrayBoundExpr(ArrayBoundExpr), + ContainerNeedsDereference(ContainerNeedsDereference), + OnlyUsedAsIndex(true), AliasDecl(nullptr), + ConfidenceLevel(Confidence::CL_Safe), NextStmtParent(nullptr), + CurrStmtParent(nullptr), ReplaceWithAliasUse(false), + AliasFromForInit(false) { + if (ContainerExpr) { + addComponent(ContainerExpr); + FoldingSetNodeID ID; + const Expr *E = ContainerExpr->IgnoreParenImpCasts(); + E->Profile(ID, *Context, true); + } +} + +bool ForLoopIndexUseVisitor::findAndVerifyUsages(const Stmt *Body) { + TraverseStmt(const_cast(Body)); + return OnlyUsedAsIndex && ContainerExpr; +} + +void ForLoopIndexUseVisitor::addComponents(const ComponentVector &Components) { + // FIXME: add sort(on ID)+unique to avoid extra work. + for (const auto &I : Components) + addComponent(I); +} + +void ForLoopIndexUseVisitor::addComponent(const Expr *E) { + FoldingSetNodeID ID; + const Expr *Node = E->IgnoreParenImpCasts(); + Node->Profile(ID, *Context, true); + DependentExprs.push_back(std::make_pair(Node, ID)); +} + +/// \brief If the unary operator is a dereference of IndexVar, include it +/// as a valid usage and prune the traversal. +/// +/// For example, if container.begin() and container.end() both return pointers +/// to int, this makes sure that the initialization for `k` is not counted as an +/// unconvertible use of the iterator `i`. +/// \code +/// for (int *i = container.begin(), *e = container.end(); i != e; ++i) { +/// int k = *i + 2; +/// } +/// \endcode +bool ForLoopIndexUseVisitor::TraverseUnaryDeref(UnaryOperator *Uop) { + // If we dereference an iterator that's actually a pointer, count the + // occurrence. + if (isDereferenceOfUop(Uop, IndexVar)) { + Usages.push_back(Usage(Uop)); + return true; + } + + return VisitorBase::TraverseUnaryOperator(Uop); +} + +/// \brief If the member expression is operator-> (overloaded or not) on +/// IndexVar, include it as a valid usage and prune the traversal. +/// +/// For example, given +/// \code +/// struct Foo { int bar(); int x; }; +/// vector v; +/// \endcode +/// the following uses will be considered convertible: +/// \code +/// for (vector::iterator i = v.begin(), e = v.end(); i != e; ++i) { +/// int b = i->bar(); +/// int k = i->x + 1; +/// } +/// \endcode +/// though +/// \code +/// for (vector::iterator i = v.begin(), e = v.end(); i != e; ++i) { +/// int k = i.insert(1); +/// } +/// for (vector::iterator i = v.begin(), e = v.end(); i != e; ++i) { +/// int b = e->bar(); +/// } +/// \endcode +/// will not. +bool ForLoopIndexUseVisitor::TraverseMemberExpr(MemberExpr *Member) { + const Expr *Base = Member->getBase(); + const DeclRefExpr *Obj = getDeclRef(Base); + const Expr *ResultExpr = Member; + QualType ExprType; + if (const auto *Call = + dyn_cast(Base->IgnoreParenImpCasts())) { + // If operator->() is a MemberExpr containing a CXXOperatorCallExpr, then + // the MemberExpr does not have the expression we want. We therefore catch + // that instance here. + // For example, if vector::iterator defines operator->(), then the + // example `i->bar()` at the top of this function is a CXXMemberCallExpr + // referring to `i->` as the member function called. We want just `i`, so + // we take the argument to operator->() as the base object. + if (Call->getOperator() == OO_Arrow) { + assert(Call->getNumArgs() == 1 && + "Operator-> takes more than one argument"); + Obj = getDeclRef(Call->getArg(0)); + ResultExpr = Obj; + ExprType = Call->getCallReturnType(*Context); + } + } + + if (Member->isArrow() && Obj && exprReferencesVariable(IndexVar, Obj)) { + if (ExprType.isNull()) + ExprType = Obj->getType(); + + assert(ExprType->isPointerType() && "Operator-> returned non-pointer type"); + // FIXME: This works around not having the location of the arrow operator. + // Consider adding OperatorLoc to MemberExpr? + SourceLocation ArrowLoc = Lexer::getLocForEndOfToken( + Base->getExprLoc(), 0, Context->getSourceManager(), + Context->getLangOpts()); + // If something complicated is happening (i.e. the next token isn't an + // arrow), give up on making this work. + if (!ArrowLoc.isInvalid()) { + Usages.push_back(Usage(ResultExpr, /*IsArrow=*/true, + SourceRange(Base->getExprLoc(), ArrowLoc))); + return true; + } + } + return TraverseStmt(Member->getBase()); +} + +/// \brief If a member function call is the at() accessor on the container with +/// IndexVar as the single argument, include it as a valid usage and prune +/// the traversal. +/// +/// Member calls on other objects will not be permitted. +/// Calls on the iterator object are not permitted, unless done through +/// operator->(). The one exception is allowing vector::at() for pseudoarrays. +bool ForLoopIndexUseVisitor::TraverseCXXMemberCallExpr( + CXXMemberCallExpr *MemberCall) { + auto *Member = + dyn_cast(MemberCall->getCallee()->IgnoreParenImpCasts()); + if (!Member) + return VisitorBase::TraverseCXXMemberCallExpr(MemberCall); + + // We specifically allow an accessor named "at" to let STL in, though + // this is restricted to pseudo-arrays by requiring a single, integer + // argument. + const IdentifierInfo *Ident = Member->getMemberDecl()->getIdentifier(); + if (Ident && Ident->isStr("at") && MemberCall->getNumArgs() == 1) { + if (isIndexInSubscriptExpr(Context, MemberCall->getArg(0), IndexVar, + Member->getBase(), ContainerExpr, + ContainerNeedsDereference)) { + Usages.push_back(Usage(MemberCall)); + return true; + } + } + + if (containsExpr(Context, &DependentExprs, Member->getBase())) + ConfidenceLevel.lowerTo(Confidence::CL_Risky); + + return VisitorBase::TraverseCXXMemberCallExpr(MemberCall); +} + +/// \brief If an overloaded operator call is a dereference of IndexVar or +/// a subscript of a the container with IndexVar as the single argument, +/// include it as a valid usage and prune the traversal. +/// +/// For example, given +/// \code +/// struct Foo { int bar(); int x; }; +/// vector v; +/// void f(Foo); +/// \endcode +/// the following uses will be considered convertible: +/// \code +/// for (vector::iterator i = v.begin(), e = v.end(); i != e; ++i) { +/// f(*i); +/// } +/// for (int i = 0; i < v.size(); ++i) { +/// int i = v[i] + 1; +/// } +/// \endcode +bool ForLoopIndexUseVisitor::TraverseCXXOperatorCallExpr( + CXXOperatorCallExpr *OpCall) { + switch (OpCall->getOperator()) { + case OO_Star: + if (isDereferenceOfOpCall(OpCall, IndexVar)) { + Usages.push_back(Usage(OpCall)); + return true; + } + break; + + case OO_Subscript: + if (OpCall->getNumArgs() != 2) + break; + if (isIndexInSubscriptExpr(Context, OpCall->getArg(1), IndexVar, + OpCall->getArg(0), ContainerExpr, + ContainerNeedsDereference)) { + Usages.push_back(Usage(OpCall)); + return true; + } + break; + + default: + break; + } + return VisitorBase::TraverseCXXOperatorCallExpr(OpCall); +} + +/// \brief If we encounter an array with IndexVar as the index of an +/// ArraySubsriptExpression, note it as a consistent usage and prune the +/// AST traversal. +/// +/// For example, given +/// \code +/// const int N = 5; +/// int arr[N]; +/// \endcode +/// This is intended to permit +/// \code +/// for (int i = 0; i < N; ++i) { /* use arr[i] */ } +/// \endcode +/// but not +/// \code +/// for (int i = 0; i < N; ++i) { /* use notArr[i] */ } +/// \endcode +/// and further checking needs to be done later to ensure that exactly one array +/// is referenced. +bool ForLoopIndexUseVisitor::TraverseArraySubscriptExpr(ArraySubscriptExpr *E) { + Expr *Arr = E->getBase(); + if (!isIndexInSubscriptExpr(E->getIdx(), IndexVar)) + return VisitorBase::TraverseArraySubscriptExpr(E); + + if ((ContainerExpr && + !areSameExpr(Context, Arr->IgnoreParenImpCasts(), + ContainerExpr->IgnoreParenImpCasts())) || + !arrayMatchesBoundExpr(Context, Arr->IgnoreImpCasts()->getType(), + ArrayBoundExpr)) { + // If we have already discovered the array being indexed and this isn't it + // or this array doesn't match, mark this loop as unconvertible. + OnlyUsedAsIndex = false; + return VisitorBase::TraverseArraySubscriptExpr(E); + } + + if (!ContainerExpr) + ContainerExpr = Arr; + + Usages.push_back(Usage(E)); + return true; +} + +/// \brief If we encounter a reference to IndexVar in an unpruned branch of the +/// traversal, mark this loop as unconvertible. +/// +/// This implements the whitelist for convertible loops: any usages of IndexVar +/// not explicitly considered convertible by this traversal will be caught by +/// this function. +/// +/// Additionally, if the container expression is more complex than just a +/// DeclRefExpr, and some part of it is appears elsewhere in the loop, lower +/// our confidence in the transformation. +/// +/// For example, these are not permitted: +/// \code +/// for (int i = 0; i < N; ++i) { printf("arr[%d] = %d", i, arr[i]); } +/// 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) +/// i.insert(0); +/// for (vector::iterator i = container.begin(), e = container.end(); +/// i != e; ++i) +/// if (i + 1 != e) +/// printf("%d", *i); +/// \endcode +/// +/// And these will raise the risk level: +/// \code +/// int arr[10][20]; +/// int l = 5; +/// for (int j = 0; j < 20; ++j) +/// int k = arr[l][j] + l; // using l outside arr[l] is considered risky +/// for (int i = 0; i < obj.getVector().size(); ++i) +/// obj.foo(10); // using `obj` is considered risky +/// \endcode +bool ForLoopIndexUseVisitor::VisitDeclRefExpr(DeclRefExpr *E) { + const ValueDecl *TheDecl = E->getDecl(); + if (areSameVariable(IndexVar, TheDecl) || areSameVariable(EndVar, TheDecl)) + OnlyUsedAsIndex = false; + if (containsExpr(Context, &DependentExprs, E)) + ConfidenceLevel.lowerTo(Confidence::CL_Risky); + return true; +} + +/// \brief If we find that another variable is created just to refer to the loop +/// element, note it for reuse as the loop variable. +/// +/// See the comments for isAliasDecl. +bool ForLoopIndexUseVisitor::VisitDeclStmt(DeclStmt *S) { + if (!AliasDecl && S->isSingleDecl() && + isAliasDecl(S->getSingleDecl(), IndexVar)) { + AliasDecl = S; + if (CurrStmtParent) { + if (isa(CurrStmtParent) || isa(CurrStmtParent) || + isa(CurrStmtParent)) + ReplaceWithAliasUse = true; + else if (isa(CurrStmtParent)) { + if (cast(CurrStmtParent)->getConditionVariableDeclStmt() == S) + ReplaceWithAliasUse = true; + else + // It's assumed S came the for loop's init clause. + AliasFromForInit = true; + } + } + } + + return true; +} + +bool ForLoopIndexUseVisitor::TraverseStmt(Stmt *S) { + // All this pointer swapping is a mechanism for tracking immediate parentage + // of Stmts. + const Stmt *OldNextParent = NextStmtParent; + CurrStmtParent = NextStmtParent; + NextStmtParent = S; + bool Result = VisitorBase::TraverseStmt(S); + NextStmtParent = OldNextParent; + return Result; +} + +std::string VariableNamer::createIndexName() { + // FIXME: Add in naming conventions to handle: + // - Uppercase/lowercase indices. + // - How to handle conflicts. + // - An interactive process for naming. + std::string IteratorName; + std::string ContainerName; + if (TheContainer) + ContainerName = TheContainer->getName().str(); + + size_t Len = ContainerName.length(); + if (Len > 1 && ContainerName[Len - 1] == 's') + IteratorName = ContainerName.substr(0, Len - 1); + else + IteratorName = "elem"; + + if (!declarationExists(IteratorName)) + return IteratorName; + + IteratorName = ContainerName + "_" + OldIndex->getName().str(); + if (!declarationExists(IteratorName)) + return IteratorName; + + IteratorName = ContainerName + "_elem"; + if (!declarationExists(IteratorName)) + return IteratorName; + + IteratorName += "_elem"; + if (!declarationExists(IteratorName)) + return IteratorName; + + IteratorName = "_elem_"; + + // Someone defeated my naming scheme... + while (declarationExists(IteratorName)) + IteratorName += "i"; + return IteratorName; +} + +/// \brief Determines whether or not the the name \a Symbol conflicts with +/// language keywords or defined macros. Also checks if the name exists in +/// LoopContext, any of its parent contexts, or any of its child statements. +/// +/// We also check to see if the same identifier was generated by this loop +/// converter in a loop nested within SourceStmt. +bool VariableNamer::declarationExists(StringRef Symbol) { + assert(Context != nullptr && "Expected an ASTContext"); + IdentifierInfo &Ident = Context->Idents.get(Symbol); + + // Check if the symbol is not an identifier (ie. is a keyword or alias). + if (!isAnyIdentifier(Ident.getTokenID())) + return true; + + // Check for conflicting macro definitions. + if (Ident.hasMacroDefinition()) + return true; + + // Determine if the symbol was generated in a parent context. + for (const Stmt *S = SourceStmt; S != nullptr; S = ReverseAST->lookup(S)) { + StmtGeneratedVarNameMap::const_iterator I = GeneratedDecls->find(S); + if (I != GeneratedDecls->end() && I->second == Symbol) + return true; + } + + // FIXME: Rather than detecting conflicts at their usages, we should check the + // parent context. + // For some reason, lookup() always returns the pair (NULL, NULL) because its + // StoredDeclsMap is not initialized (i.e. LookupPtr.getInt() is false inside + // of DeclContext::lookup()). Why is this? + + // Finally, determine if the symbol was used in the loop or a child context. + DeclFinderASTVisitor DeclFinder(Symbol, GeneratedDecls); + return DeclFinder.findUsages(SourceStmt); +} + +} // namespace modernize +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h new file mode 100644 index 000000000000..47da6656928e --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h @@ -0,0 +1,420 @@ +//===--- LoopConvertUtils.h - clang-tidy ------------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_LOOP_CONVERT_UTILS_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_LOOP_CONVERT_UTILS_H + +#include "clang/AST/ASTContext.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" +#include "clang/Tooling/Refactoring.h" + +namespace clang { +namespace tidy { +namespace modernize { + +enum LoopFixerKind { LFK_Array, LFK_Iterator, LFK_PseudoArray }; + +/// A map used to walk the AST in reverse: maps child Stmt to parent Stmt. +typedef llvm::DenseMap StmtParentMap; + +/// A map used to walk the AST in reverse: +/// maps VarDecl to the to parent DeclStmt. +typedef llvm::DenseMap + DeclParentMap; + +/// A map used to track which variables have been removed by a refactoring pass. +/// It maps the parent ForStmt to the removed index variable's VarDecl. +typedef llvm::DenseMap + ReplacedVarsMap; + +/// A map used to remember the variable names generated in a Stmt +typedef llvm::DenseMap + StmtGeneratedVarNameMap; + +/// A vector used to store the AST subtrees of an Expr. +typedef llvm::SmallVector ComponentVector; + +/// \brief Class used build the reverse AST properties needed to detect +/// name conflicts and free variables. +class StmtAncestorASTVisitor + : public clang::RecursiveASTVisitor { +public: + StmtAncestorASTVisitor() { StmtStack.push_back(nullptr); } + + /// \brief Run the analysis on the TranslationUnitDecl. + /// + /// In case we're running this analysis multiple times, don't repeat the work. + void gatherAncestors(const clang::TranslationUnitDecl *T) { + if (StmtAncestors.empty()) + TraverseDecl(const_cast(T)); + } + + /// Accessor for StmtAncestors. + const StmtParentMap &getStmtToParentStmtMap() { return StmtAncestors; } + + /// Accessor for DeclParents. + const DeclParentMap &getDeclToParentStmtMap() { return DeclParents; } + + friend class clang::RecursiveASTVisitor; + +private: + StmtParentMap StmtAncestors; + DeclParentMap DeclParents; + llvm::SmallVector StmtStack; + + bool TraverseStmt(clang::Stmt *Statement); + bool VisitDeclStmt(clang::DeclStmt *Statement); +}; + +/// Class used to find the variables and member expressions on which an +/// arbitrary expression depends. +class ComponentFinderASTVisitor + : public clang::RecursiveASTVisitor { +public: + ComponentFinderASTVisitor() {} + + /// Find the components of an expression and place them in a ComponentVector. + void findExprComponents(const clang::Expr *SourceExpr) { + TraverseStmt(const_cast(SourceExpr)); + } + + /// Accessor for Components. + const ComponentVector &getComponents() { return Components; } + + friend class clang::RecursiveASTVisitor; + +private: + ComponentVector Components; + + bool VisitDeclRefExpr(clang::DeclRefExpr *E); + bool VisitMemberExpr(clang::MemberExpr *Member); +}; + +/// Class used to determine if an expression is dependent on a variable declared +/// inside of the loop where it would be used. +class DependencyFinderASTVisitor + : public clang::RecursiveASTVisitor { +public: + DependencyFinderASTVisitor(const StmtParentMap *StmtParents, + const DeclParentMap *DeclParents, + const ReplacedVarsMap *ReplacedVars, + const clang::Stmt *ContainingStmt) + : StmtParents(StmtParents), DeclParents(DeclParents), + ContainingStmt(ContainingStmt), ReplacedVars(ReplacedVars) {} + + /// \brief Run the analysis on Body, and return true iff the expression + /// depends on some variable declared within ContainingStmt. + /// + /// This is intended to protect against hoisting the container expression + /// outside of an inner context if part of that expression is declared in that + /// inner context. + /// + /// For example, + /// \code + /// const int N = 10, M = 20; + /// int arr[N][M]; + /// int getRow(); + /// + /// for (int i = 0; i < M; ++i) { + /// int k = getRow(); + /// printf("%d:", arr[k][i]); + /// } + /// \endcode + /// At first glance, this loop looks like it could be changed to + /// \code + /// for (int elem : arr[k]) { + /// int k = getIndex(); + /// printf("%d:", elem); + /// } + /// \endcode + /// But this is malformed, since `k` is used before it is defined! + /// + /// In order to avoid this, this class looks at the container expression + /// `arr[k]` and decides whether or not it contains a sub-expression declared + /// within the the loop body. + bool dependsOnInsideVariable(const clang::Stmt *Body) { + DependsOnInsideVariable = false; + TraverseStmt(const_cast(Body)); + return DependsOnInsideVariable; + } + + friend class clang::RecursiveASTVisitor; + +private: + const StmtParentMap *StmtParents; + const DeclParentMap *DeclParents; + const clang::Stmt *ContainingStmt; + const ReplacedVarsMap *ReplacedVars; + bool DependsOnInsideVariable; + + bool VisitVarDecl(clang::VarDecl *V); + bool VisitDeclRefExpr(clang::DeclRefExpr *D); +}; + +/// Class used to determine if any declarations used in a Stmt would conflict +/// with a particular identifier. This search includes the names that don't +/// actually appear in the AST (i.e. created by a refactoring tool) by including +/// a map from Stmts to generated names associated with those stmts. +class DeclFinderASTVisitor + : public clang::RecursiveASTVisitor { +public: + DeclFinderASTVisitor(const std::string &Name, + const StmtGeneratedVarNameMap *GeneratedDecls) + : Name(Name), GeneratedDecls(GeneratedDecls), Found(false) {} + + /// Attempts to find any usages of variables name Name in Body, returning + /// true when it is used in Body. This includes the generated loop variables + /// of ForStmts which have already been transformed. + bool findUsages(const clang::Stmt *Body) { + Found = false; + TraverseStmt(const_cast(Body)); + return Found; + } + + friend class clang::RecursiveASTVisitor; + +private: + std::string Name; + /// GeneratedDecls keeps track of ForStmts which have been transformed, + /// mapping each modified ForStmt to the variable generated in the loop. + const StmtGeneratedVarNameMap *GeneratedDecls; + bool Found; + + bool VisitForStmt(clang::ForStmt *F); + bool VisitNamedDecl(clang::NamedDecl *D); + bool VisitDeclRefExpr(clang::DeclRefExpr *D); + bool VisitTypeLoc(clang::TypeLoc TL); +}; + +/// \brief The information needed to describe a valid convertible usage +/// of an array index or iterator. +struct Usage { + const Expr *E; + bool IsArrow; + SourceRange Range; + + explicit Usage(const Expr *E) + : E(E), IsArrow(false), Range(E->getSourceRange()) {} + Usage(const Expr *E, bool IsArrow, SourceRange Range) + : E(E), IsArrow(IsArrow), Range(std::move(Range)) {} +}; + +/// \brief A class to encapsulate lowering of the tool's confidence level. +class Confidence { +public: + enum Level { + // Transformations that are likely to change semantics. + CL_Risky, + + // Transformations that might change semantics. + CL_Reasonable, + + // Transformations that will not change semantics. + CL_Safe + }; + /// \brief Initialize confidence level. + explicit Confidence(Confidence::Level Level) : CurrentLevel(Level) {} + + /// \brief Lower the internal confidence level to Level, but do not raise it. + void lowerTo(Confidence::Level Level) { + CurrentLevel = std::min(Level, CurrentLevel); + } + + /// \brief Return the internal confidence level. + Level getLevel() const { return CurrentLevel; } + +private: + Level CurrentLevel; +}; + +// The main computational result of ForLoopIndexVisitor. +typedef llvm::SmallVector UsageResult; + +// General functions used by ForLoopIndexUseVisitor and LoopConvertCheck. +const Expr *digThroughConstructors(const Expr *E); +bool areSameExpr(ASTContext *Context, const Expr *First, const Expr *Second); +const DeclRefExpr *getDeclRef(const Expr *E); +bool areSameVariable(const ValueDecl *First, const ValueDecl *Second); + +/// \brief Discover usages of expressions consisting of index or iterator +/// access. +/// +/// Given an index variable, recursively crawls a for loop to discover if the +/// index variable is used in a way consistent with range-based for loop access. +class ForLoopIndexUseVisitor + : public RecursiveASTVisitor { +public: + ForLoopIndexUseVisitor(ASTContext *Context, const VarDecl *IndexVar, + const VarDecl *EndVar, const Expr *ContainerExpr, + const Expr *ArrayBoundExpr, + bool ContainerNeedsDereference); + + /// \brief Finds all uses of IndexVar in Body, placing all usages in Usages, + /// and returns true if IndexVar was only used in a way consistent with a + /// range-based for loop. + /// + /// The general strategy is to reject any DeclRefExprs referencing IndexVar, + /// with the exception of certain acceptable patterns. + /// For arrays, the DeclRefExpr for IndexVar must appear as the index of an + /// ArraySubscriptExpression. Iterator-based loops may dereference + /// IndexVar or call methods through operator-> (builtin or overloaded). + /// Array-like containers may use IndexVar as a parameter to the at() member + /// function and in overloaded operator[]. + bool findAndVerifyUsages(const Stmt *Body); + + /// \brief Add a set of components that we should consider relevant to the + /// container. + void addComponents(const ComponentVector &Components); + + /// \brief Accessor for Usages. + const UsageResult &getUsages() const { return Usages; } + + /// \brief Get the container indexed by IndexVar, if any. + const Expr *getContainerIndexed() const { return ContainerExpr; } + + /// \brief Returns the statement declaring the variable created as an alias + /// for the loop element, if any. + const DeclStmt *getAliasDecl() const { return AliasDecl; } + + /// \brief Accessor for ConfidenceLevel. + Confidence::Level getConfidenceLevel() const { + return ConfidenceLevel.getLevel(); + } + + /// \brief Indicates if the alias declaration was in a place where it cannot + /// simply be removed but rather replaced with a use of the alias variable. + /// For example, variables declared in the condition of an if, switch, or for + /// stmt. + bool aliasUseRequired() const { return ReplaceWithAliasUse; } + + /// \brief Indicates if the alias declaration came from the init clause of a + /// nested for loop. SourceRanges provided by Clang for DeclStmts in this + /// case need to be adjusted. + bool aliasFromForInit() const { return AliasFromForInit; } + +private: + /// Typedef used in CRTP functions. + typedef RecursiveASTVisitor VisitorBase; + friend class RecursiveASTVisitor; + + /// Overriden methods for RecursiveASTVisitor's traversal. + bool TraverseArraySubscriptExpr(ArraySubscriptExpr *E); + bool TraverseCXXMemberCallExpr(CXXMemberCallExpr *MemberCall); + bool TraverseCXXOperatorCallExpr(CXXOperatorCallExpr *OpCall); + bool TraverseMemberExpr(MemberExpr *Member); + bool TraverseUnaryDeref(UnaryOperator *Uop); + bool VisitDeclRefExpr(DeclRefExpr *E); + bool VisitDeclStmt(DeclStmt *S); + bool TraverseStmt(Stmt *S); + + /// \brief Add an expression to the list of expressions on which the container + /// expression depends. + void addComponent(const Expr *E); + + // Input member variables: + ASTContext *Context; + /// The index variable's VarDecl. + const VarDecl *IndexVar; + /// The loop's 'end' variable, which cannot be mentioned at all. + const VarDecl *EndVar; + /// The Expr which refers to the container. + const Expr *ContainerExpr; + /// The Expr which refers to the terminating condition for array-based loops. + const Expr *ArrayBoundExpr; + bool ContainerNeedsDereference; + + // Output member variables: + /// A container which holds all usages of IndexVar as the index of + /// ArraySubscriptExpressions. + UsageResult Usages; + bool OnlyUsedAsIndex; + /// The DeclStmt for an alias to the container element. + const DeclStmt *AliasDecl; + Confidence ConfidenceLevel; + /// \brief A list of expressions on which ContainerExpr depends. + /// + /// If any of these expressions are encountered outside of an acceptable usage + /// of the loop element, lower our confidence level. + llvm::SmallVector, 16> + DependentExprs; + + /// The parent-in-waiting. Will become the real parent once we traverse down + /// one level in the AST. + const Stmt *NextStmtParent; + /// The actual parent of a node when Visit*() calls are made. Only the + /// parentage of DeclStmt's to possible iteration/selection statements is of + /// importance. + const Stmt *CurrStmtParent; + + /// \see aliasUseRequired(). + bool ReplaceWithAliasUse; + /// \see aliasFromForInit(). + bool AliasFromForInit; +}; + +struct TUTrackingInfo { + /// \brief Reset and initialize per-TU tracking information. + /// + /// Must be called before using container accessors. + TUTrackingInfo() : ParentFinder(new StmtAncestorASTVisitor) {} + + StmtAncestorASTVisitor &getParentFinder() { return *ParentFinder; } + StmtGeneratedVarNameMap &getGeneratedDecls() { return GeneratedDecls; } + ReplacedVarsMap &getReplacedVars() { return ReplacedVars; } + +private: + std::unique_ptr ParentFinder; + StmtGeneratedVarNameMap GeneratedDecls; + ReplacedVarsMap ReplacedVars; +}; + +/// \brief Create names for generated variables within a particular statement. +/// +/// VariableNamer uses a DeclContext as a reference point, checking for any +/// conflicting declarations higher up in the context or within SourceStmt. +/// It creates a variable name using hints from a source container and the old +/// index, if they exist. +class VariableNamer { +public: + VariableNamer(StmtGeneratedVarNameMap *GeneratedDecls, + const StmtParentMap *ReverseAST, const clang::Stmt *SourceStmt, + const clang::VarDecl *OldIndex, + const clang::VarDecl *TheContainer, + const clang::ASTContext *Context) + : GeneratedDecls(GeneratedDecls), ReverseAST(ReverseAST), + SourceStmt(SourceStmt), OldIndex(OldIndex), TheContainer(TheContainer), + Context(Context) {} + + /// \brief Generate a new index name. + /// + /// Generates the name to be used for an inserted iterator. It relies on + /// declarationExists() to determine that there are no naming conflicts, and + /// tries to use some hints from the container name and the old index name. + std::string createIndexName(); + +private: + StmtGeneratedVarNameMap *GeneratedDecls; + const StmtParentMap *ReverseAST; + const clang::Stmt *SourceStmt; + const clang::VarDecl *OldIndex; + const clang::VarDecl *TheContainer; + const clang::ASTContext *Context; + + // Determine whether or not a declaration that would conflict with Symbol + // exists in an outer context or in any statement contained in SourceStmt. + bool declarationExists(llvm::StringRef Symbol); +}; + +} // namespace modernize +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_LOOP_CONVERT_UTILS_H diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp index 5f25832e8a66..962245b2a081 100644 --- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "LoopConvertCheck.h" #include "PassByValueCheck.h" using namespace clang::ast_matchers; @@ -21,12 +22,14 @@ namespace modernize { class ModernizeModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck("modernize-loop-convert"); CheckFactories.registerCheck("modernize-pass-by-value"); } ClangTidyOptions getModuleOptions() override { ClangTidyOptions Options; auto &Opts = Options.CheckOptions; + Opts["modernize-loop-convert.MinConfidence"] = "reasonable"; Opts["modernize-pass-by-value.IncludeStyle"] = "llvm"; // Also: "google". return Options; } 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 new file mode 100644 index 000000000000..9dd04f42b5ab --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/Inputs/modernize-loop-convert/structures.h @@ -0,0 +1,179 @@ +#ifndef STRUCTURES_H +#define STRUCTURES_H + +extern "C" { +extern int printf(const char *restrict, ...); +} + +struct Val {int x; void g(); }; + +struct MutableVal { + void constFun(int) const; + void nonConstFun(int, int); + void constFun(MutableVal &) const; + void constParamFun(const MutableVal &) const; + void nonConstParamFun(const MutableVal &); + int x; +}; + +struct S { + typedef MutableVal *iterator; + typedef const MutableVal *const_iterator; + const_iterator begin() const; + const_iterator end() const; + iterator begin(); + iterator end(); +}; + +struct T { + struct iterator { + int& operator*(); + const int& operator*()const; + iterator& operator ++(); + bool operator!=(const iterator &other); + void insert(int); + int x; + }; + iterator begin(); + iterator end(); +}; + +struct U { + struct iterator { + Val& operator*(); + const Val& operator*()const; + iterator& operator ++(); + bool operator!=(const iterator &other); + Val *operator->(); + }; + iterator begin(); + iterator end(); + int x; +}; + +struct X { + S s; + T t; + U u; + S getS(); +}; + +template +class dependent{ + public: + struct iterator_base { + const ElemType& operator*()const; + iterator_base& operator ++(); + bool operator!=(const iterator_base &other) const; + const ElemType *operator->() const; + }; + + struct iterator : iterator_base { + ElemType& operator*(); + iterator& operator ++(); + ElemType *operator->(); + }; + + typedef iterator_base const_iterator; + const_iterator begin() const; + const_iterator end() const; + iterator begin(); + iterator end(); + unsigned size() const; + ElemType & operator[](unsigned); + const ElemType & operator[](unsigned) const; + ElemType & at(unsigned); + const ElemType & at(unsigned) const; + + // Intentionally evil. + dependent operator*(); + + void foo(); + void constFoo() const; +}; + +template +class doublyDependent{ + public: + struct Value { + First first; + Second second; + }; + + struct iterator_base { + const Value& operator*()const; + iterator_base& operator ++(); + bool operator!=(const iterator_base &other) const; + const Value *operator->() const; + }; + + struct iterator : iterator_base { + Value& operator*(); + Value& operator ++(); + Value *operator->(); + }; + + typedef iterator_base const_iterator; + const_iterator begin() const; + const_iterator end() const; + iterator begin(); + iterator end(); +}; + +template +class transparent { + public: + Contained *at(); + Contained *operator->(); + Contained operator*(); +}; + +template +struct Nested { + typedef IteratorType* iterator; + typedef const IteratorType* const_iterator; + IteratorType *operator->(); + IteratorType operator*(); + iterator begin(); + iterator end(); + const_iterator begin() const; + const_iterator end() const; +}; + +// Like llvm::SmallPtrSet, the iterator has a dereference operator that returns +// by value instead of by reference. +template +struct PtrSet { + struct iterator { + bool operator!=(const iterator &other) const; + const T operator*(); + iterator &operator++(); + }; + iterator begin() const; + iterator end() const; +}; + +template +struct TypedefDerefContainer { + struct iterator { + typedef T &deref_type; + bool operator!=(const iterator &other) const; + deref_type operator*(); + iterator &operator++(); + }; + iterator begin() const; + iterator end() const; +}; + +template +struct RValueDerefContainer { + struct iterator { + typedef T &&deref_type; + bool operator!=(const iterator &other) const; + deref_type operator*(); + iterator &operator++(); + }; + iterator begin() const; + iterator end() const; +}; +#endif // STRUCTURES_H 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 new file mode 100644 index 000000000000..9c97cfa5cf33 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp @@ -0,0 +1,548 @@ +// RUN: $(dirname %s)/check_clang_tidy.sh %s modernize-loop-convert %t -- -std=c++11 -I %S/Inputs/modernize-loop-convert +// REQUIRES: shell + +#include "structures.h" + +namespace Array { + +const int N = 6; +const int NMinusOne = N - 1; +int arr[N] = {1, 2, 3, 4, 5, 6}; +int (*pArr)[N] = &arr; + +void f() { + int sum = 0; + + for (int i = 0; i < N; ++i) { + sum += arr[i]; + 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-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]); + sum += arr[i] + 2; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : arr) + // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", elem); + // CHECK-FIXES-NEXT: sum += elem + 2; + + for (int i = 0; i < N; ++i) { + int x = arr[i]; + int y = arr[i] + 2; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : arr) + // CHECK-FIXES-NEXT: int x = elem; + // CHECK-FIXES-NEXT: int y = elem + 2; + + for (int i = 0; i < N; ++i) { + int x = N; + x = arr[i]; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : arr) + // CHECK-FIXES-NEXT: int x = N; + // CHECK-FIXES-NEXT: x = elem; + + for (int i = 0; i < N; ++i) { + arr[i] += 1; + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // 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; + arr[i]++; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : arr) + // CHECK-FIXES-NEXT: int x = elem + 2; + // CHECK-FIXES-NEXT: elem++; + + for (int i = 0; i < N; ++i) { + arr[i] = 4 + arr[i]; + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : arr) + // CHECK-FIXES-NEXT: elem = 4 + elem; + + for (int i = 0; i < NMinusOne + 1; ++i) { + sum += arr[i]; + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // 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]); + sum += arr[i] + 2; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : arr) + // CHECK-FIXES-NEXT: printf("Fibonacci number %d has address %p\n", elem, &elem); + // CHECK-FIXES-NEXT: sum += elem + 2; + + Val teas[N]; + for (int i = 0; i < N; ++i) { + teas[i].g(); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & tea : teas) { + // CHECK-FIXES-NEXT: tea.g(); + // CHECK-FIXES-NEXT: } +} + +struct HasArr { + int Arr[N]; + Val ValArr[N]; + void implicitThis() { + for (int i = 0; i < N; ++i) { + printf("%d", Arr[i]); + } + // CHECK-MESSAGES: :[[@LINE-3]]:5: warning: use range-based for loop instead + // 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-NEXT: printf("%d", elem.x); + // CHECK-FIXES-NEXT: } + } + + void explicitThis() { + for (int i = 0; i < N; ++i) { + 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-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-NEXT: printf("%d", elem.x); + // CHECK-FIXES-NEXT: } + } +}; + +// Loops whose bounds are value-dependent shold not be converted. +template +void dependentExprBound() { + for (int i = 0; i < N; ++i) + arr[i] = 0; +} +template void dependentExprBound<20>(); + +void memberFunctionPointer() { + Val v; + void (Val::*mfpArr[N])(void) = {&Val::g}; + for (int i = 0; i < N; ++i) + (v.*mfpArr[i])(); + // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : mfpArr) + // CHECK-FIXES-NEXT: (v.*elem)(); +} + +} // namespace Array + +namespace Iterator { + +void f() { + /// begin()/end() - based for loops here: + T t; + for (T::iterator it = t.begin(), e = t.end(); it != e; ++it) { + printf("I found %d\n", *it); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : t) + // CHECK-FIXES-NEXT: printf("I found %d\n", elem); + + T *pt; + for (T::iterator it = pt->begin(), e = pt->end(); it != e; ++it) { + printf("I found %d\n", *it); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : *pt) + // CHECK-FIXES-NEXT: printf("I found %d\n", elem); + + S s; + for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) { + printf("s has value %d\n", (*it).x); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : s) + // CHECK-FIXES-NEXT: printf("s has value %d\n", (elem).x); + + S *ps; + for (S::iterator it = ps->begin(), e = ps->end(); it != e; ++it) { + printf("s has value %d\n", (*it).x); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & p : *ps) + // CHECK-FIXES-NEXT: printf("s has value %d\n", (p).x); + + for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) { + printf("s has value %d\n", it->x); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : s) + // CHECK-FIXES-NEXT: printf("s has value %d\n", elem.x); + + for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) { + it->x = 3; + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : s) + // CHECK-FIXES-NEXT: elem.x = 3; + + for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) { + (*it).x = 3; + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : s) + // CHECK-FIXES-NEXT: (elem).x = 3; + + for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) { + it->nonConstFun(4, 5); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : s) + // CHECK-FIXES-NEXT: elem.nonConstFun(4, 5); + + U u; + for (U::iterator it = u.begin(), e = u.end(); it != e; ++it) { + printf("s has value %d\n", it->x); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : u) + // CHECK-FIXES-NEXT: printf("s has value %d\n", elem.x); + + for (U::iterator it = u.begin(), e = u.end(); it != e; ++it) { + printf("s has value %d\n", (*it).x); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : u) + // CHECK-FIXES-NEXT: printf("s has value %d\n", (elem).x); + + U::iterator A; + for (U::iterator i = u.begin(), e = u.end(); i != e; ++i) + int k = A->x + i->x; + // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : u) + // CHECK-FIXES-NEXT: int k = A->x + elem.x; + + dependent v; + for (dependent::iterator it = v.begin(), e = v.end(); + it != e; ++it) { + 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-NEXT: printf("Fibonacci number is %d\n", elem); + + for (dependent::iterator it(v.begin()), e = v.end(); + it != e; ++it) { + 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-NEXT: printf("Fibonacci number is %d\n", elem); + + doublyDependent intmap; + for (doublyDependent::iterator it = intmap.begin(), e = intmap.end(); + it != e; ++it) { + printf("intmap[%d] = %d", it->first, it->second); + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : intmap) + // CHECK-FIXES: printf("intmap[%d] = %d", elem.first, elem.second); + + // PtrSet's iterator dereferences by value so auto & can't be used. + { + PtrSet int_ptrs; + for (PtrSet::iterator I = int_ptrs.begin(), + E = int_ptrs.end(); + I != E; ++I) { + } + // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use range-based for loop instead + // CHECK-FIXES: for (auto && int_ptr : int_ptrs) { + } + + // This container uses an iterator where the derefence type is a typedef of + // a reference type. Make sure non-const auto & is still used. A failure here + // means canonical types aren't being tested. + { + TypedefDerefContainer int_ptrs; + for (TypedefDerefContainer::iterator I = int_ptrs.begin(), + E = int_ptrs.end(); + I != E; ++I) { + } + // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & int_ptr : int_ptrs) { + } + + { + // Iterators returning an rvalue reference should disqualify the loop from + // transformation. + RValueDerefContainer container; + for (RValueDerefContainer::iterator I = container.begin(), + E = container.end(); + I != E; ++I) { + } + // CHECK-FIXES: for (RValueDerefContainer::iterator I = container.begin(), + // CHECK-FIXES-NEXT: E = container.end(); + // CHECK-FIXES-NEXT: I != E; ++I) { + } +} + +// Tests to verify the proper use of auto where the init variable type and the +// initializer type differ or are mostly the same except for const qualifiers. +void different_type() { + // s.begin() returns a type 'iterator' which is just a non-const pointer and + // differs from const_iterator only on the const qualification. + S s; + for (S::const_iterator it = s.begin(), e = s.end(); it != e; ++it) { + printf("s has value %d\n", (*it).x); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (const auto & elem : s) + // CHECK-FIXES-NEXT: printf("s has value %d\n", (elem).x); + + S *ps; + for (S::const_iterator it = ps->begin(), e = ps->end(); it != e; ++it) { + printf("s has value %d\n", (*it).x); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (const auto & p : *ps) + // CHECK-FIXES-NEXT: printf("s has value %d\n", (p).x); + + // v.begin() returns a user-defined type 'iterator' which, since it's + // different from const_iterator, disqualifies these loops from + // transformation. + dependent v; + for (dependent::const_iterator it = v.begin(), e = v.end(); + it != e; ++it) { + printf("Fibonacci number is %d\n", *it); + } + // CHECK-FIXES: for (dependent::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::const_iterator it(v.begin()), e = v.end(); + it != e; ++it) { + printf("Fibonacci number is %d\n", *it); + } + // CHECK-FIXES: for (dependent::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. +// If member calls are made to 'this' within the loop, the transform becomes +// risky as these calls may affect state that affects the loop. +class C { +public: + typedef MutableVal *iterator; + typedef const MutableVal *const_iterator; + + iterator begin(); + iterator end(); + const_iterator begin() const; + const_iterator end() const; + + void doSomething(); + void doSomething() const; + + void doLoop() { + for (iterator I = begin(), E = end(); I != E; ++I) { + } + // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : *this) { + + for (iterator I = C::begin(), E = C::end(); I != E; ++I) { + } + // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : *this) { + + for (iterator I = begin(), E = end(); I != E; ++I) { + doSomething(); + } + + for (iterator I = begin(); I != end(); ++I) { + } + // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : *this) { + + for (iterator I = begin(); I != end(); ++I) { + doSomething(); + } + } + + void doLoop() const { + for (const_iterator I = begin(), E = end(); I != E; ++I) { + } + // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : *this) { + + for (const_iterator I = C::begin(), E = C::end(); I != E; ++I) { + } + // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : *this) { + + for (const_iterator I = begin(), E = end(); I != E; ++I) { + doSomething(); + } + } +}; + +class C2 { +public: + typedef MutableVal *iterator; + + iterator begin() const; + iterator end() const; + + 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) { + } + // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : *this) { + } +}; + +} // namespace Iterator + +namespace PseudoArray { + +const int N = 6; +dependent v; +dependent *pv; + +transparent> cv; + +void f() { + int sum = 0; + for (int i = 0, e = v.size(); i < e; ++i) { + printf("Fibonacci number is %d\n", v[i]); + sum += v[i] + 2; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : v) + // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", elem); + // CHECK-FIXES-NEXT: sum += elem + 2; + + for (int i = 0, e = v.size(); i < e; ++i) { + printf("Fibonacci number is %d\n", v.at(i)); + sum += v.at(i) + 2; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : v) + // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", elem); + // CHECK-FIXES-NEXT: sum += elem + 2; + + for (int i = 0, e = pv->size(); i < e; ++i) { + printf("Fibonacci number is %d\n", pv->at(i)); + sum += pv->at(i) + 2; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : *pv) + // 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 < pv->size(); ++i) { + printf("Fibonacci number is %d\n", (*pv).at(i)); + sum += (*pv)[i] + 2; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : *pv) + // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", elem); + // CHECK-FIXES-NEXT: sum += elem + 2; + + for (int i = 0; i < cv->size(); ++i) { + printf("Fibonacci number is %d\n", cv->at(i)); + sum += cv->at(i) + 2; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : *cv) + // 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 (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) +} + +struct NoBeginEnd { + unsigned size() const; +}; + +struct NoConstBeginEnd { + NoConstBeginEnd(); + unsigned size() const; + unsigned begin(); + unsigned end(); +}; + +struct ConstBeginEnd { + ConstBeginEnd(); + unsigned size() const; + unsigned begin() const; + unsigned end() 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) { + } + + const NoConstBeginEnd const_NCBE; + for (unsigned i = 0, e = const_NCBE.size(); i < e; ++i) { + } + + ConstBeginEnd CBE; + 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) { + + 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) { +} + +} // namespace PseudoArray 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 new file mode 100644 index 000000000000..67b40d607086 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp @@ -0,0 +1,608 @@ +// RUN: $(dirname %s)/check_clang_tidy.sh %s modernize-loop-convert %t -- -std=c++11 -I %S/Inputs/modernize-loop-convert +// REQUIRES: shell + +#include "structures.h" + +namespace Dependency { + +void f() { + const int N = 6; + const int M = 8; + int arr[N][M]; + + for (int i = 0; i < N; ++i) { + int a = 0; + 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-NEXT: int a = 0; + // CHECK-FIXES-NEXT: int b = elem[a]; + // CHECK-FIXES-NEXT: } + + for (int j = 0; j < M; ++j) { + int a = 0; + int b = arr[a][j]; + } +} + +} // namespace Dependency + +namespace NamingAlias { + +const int N = 10; + +Val Arr[N]; +dependent v; +dependent *pv; +Val &func(Val &); +void sideEffect(int); + +void aliasing() { + // If the loop container is only used for a declaration of a temporary + // variable to hold each element, we can name the new variable for the + // converted range-based loop as the temporary variable's name. + + // In the following case, "t" is used as a temporary variable to hold each + // element, and thus we consider the name "t" aliased to the loop. + // The extra blank braces are left as a placeholder for after the variable + // declaration is deleted. + for (int i = 0; i < N; ++i) { + Val &t = Arr[i]; + {} + int y = t.x; + } + // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & t : Arr) + // CHECK-FIXES-NOT: Val &{{[a-z_]+}} = + // CHECK-FIXES: {} + // CHECK-FIXES-NEXT: int y = t.x; + + // The container was not only used to initialize a temporary loop variable for + // the container's elements, so we do not alias the new loop variable. + for (int i = 0; i < N; ++i) { + Val &t = Arr[i]; + int y = t.x; + int z = Arr[i].x + t.x; + } + // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : Arr) + // CHECK-FIXES-NEXT: Val &t = elem; + // CHECK-FIXES-NEXT: int y = t.x; + // CHECK-FIXES-NEXT: int z = elem.x + t.x; + + for (int i = 0; i < N; ++i) { + Val t = Arr[i]; + int y = t.x; + int z = Arr[i].x + t.x; + } + // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : Arr) + // CHECK-FIXES-NEXT: Val t = elem; + // CHECK-FIXES-NEXT: int y = t.x; + // CHECK-FIXES-NEXT: int z = elem.x + t.x; + + // The same for pseudo-arrays like std::vector (or here dependent) + // which provide a subscript operator[]. + for (int i = 0; i < v.size(); ++i) { + Val &t = v[i]; + {} + int y = t.x; + } + // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & t : v) + // CHECK-FIXES: {} + // CHECK-FIXES-NEXT: int y = t.x; + + // The same with a call to at() + for (int i = 0; i < pv->size(); ++i) { + Val &t = pv->at(i); + {} + int y = t.x; + } + // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & t : *pv) + // CHECK-FIXES: {} + // CHECK-FIXES-NEXT: int y = t.x; + + for (int i = 0; i < N; ++i) { + Val &t = func(Arr[i]); + int y = t.x; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : Arr) + // CHECK-FIXES-NEXT: Val &t = func(elem); + // CHECK-FIXES-NEXT: int y = t.x; + + int IntArr[N]; + for (unsigned i = 0; i < N; ++i) { + if (int alias = IntArr[i]) { + sideEffect(alias); + } + } + // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto alias : IntArr) + // CHECK-FIXES-NEXT: if (alias) { + + for (unsigned i = 0; i < N; ++i) { + while (int alias = IntArr[i]) { + sideEffect(alias); + } + } + // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto alias : IntArr) + // CHECK-FIXES-NEXT: while (alias) { + + for (unsigned i = 0; i < N; ++i) { + switch (int alias = IntArr[i]) { + default: + sideEffect(alias); + } + } + // CHECK-MESSAGES: :[[@LINE-6]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto alias : IntArr) + // CHECK-FIXES-NEXT: switch (alias) { + + for (unsigned i = 0; i < N; ++i) { + for (int alias = IntArr[i]; alias < N; ++alias) { + sideEffect(alias); + } + } + // 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) { + + for (unsigned i = 0; i < N; ++i) { + for (unsigned j = 0; int alias = IntArr[i]; ++j) { + sideEffect(alias); + } + } + // 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) { +} + +void refs_and_vals() { + // The following tests check that the transform correctly preserves the + // reference or value qualifiers of the aliased variable. That is, if the + // variable was declared as a value, the loop variable will be declared as a + // value and vice versa for references. + + S s; + const S s_const = s; + + for (S::const_iterator it = s_const.begin(); it != s_const.end(); ++it) { + MutableVal alias = *it; + {} + alias.x = 0; + } + // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto alias : s_const) + // CHECK-FIXES-NOT: MutableVal {{[a-z_]+}} = + // CHECK-FIXES: {} + // CHECK-FIXES-NEXT: alias.x = 0; + + for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) { + MutableVal alias = *it; + {} + alias.x = 0; + } + // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto alias : s) + // CHECK-FIXES-NOT: MutableVal {{[a-z_]+}} = + // CHECK-FIXES: {} + // CHECK-FIXES-NEXT: alias.x = 0; + + for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) { + MutableVal &alias = *it; + {} + alias.x = 0; + } + // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & alias : s) + // CHECK-FIXES-NOT: MutableVal &{{[a-z_]+}} = + // CHECK-FIXES: {} + // CHECK-FIXES-NEXT: alias.x = 0; +} + +} // namespace NamingAlias + +namespace NamingConlict { + +#define MAX(a, b) (a > b) ? a : b +#define DEF 5 + +const int N = 10; +int nums[N]; +int sum = 0; + +namespace ns { +struct st { + int x; +}; +} + +void sameNames() { + int num = 0; + for (int i = 0; i < N; ++i) { + printf("Fibonacci number is %d\n", nums[i]); + sum += nums[i] + 2 + num; + (void)nums[i]; + } + // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & nums_i : nums) + // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", nums_i); + // CHECK-FIXES-NEXT: sum += nums_i + 2 + num; + // CHECK-FIXES-NOT: (void) num; +} + +void macroConflict() { + S MAXs; + for (S::iterator it = MAXs.begin(), e = MAXs.end(); it != e; ++it) { + printf("s has value %d\n", (*it).x); + printf("Max of 3 and 5: %d\n", MAX(3, 5)); + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & MAXs_it : MAXs) + // CHECK-FIXES-NEXT: printf("s has value %d\n", (MAXs_it).x); + // CHECK-FIXES-NEXT: printf("Max of 3 and 5: %d\n", MAX(3, 5)); + + for (S::const_iterator it = MAXs.begin(), e = MAXs.end(); it != e; ++it) { + printf("s has value %d\n", (*it).x); + printf("Max of 3 and 5: %d\n", MAX(3, 5)); + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (const auto & MAXs_it : MAXs) + // CHECK-FIXES-NEXT: printf("s has value %d\n", (MAXs_it).x); + // CHECK-FIXES-NEXT: printf("Max of 3 and 5: %d\n", MAX(3, 5)); + + T DEFs; + for (T::iterator it = DEFs.begin(), e = DEFs.end(); it != e; ++it) { + if (*it == DEF) { + printf("I found %d\n", *it); + } + } + // 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: printf("I found %d\n", DEFs_it); +} + +void keywordConflict() { + T ints; + for (T::iterator it = ints.begin(), e = ints.end(); it != e; ++it) { + *it = 5; + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & ints_it : ints) + // CHECK-FIXES-NEXT: ints_it = 5; + + U __FUNCTION__s; + for (U::iterator it = __FUNCTION__s.begin(), e = __FUNCTION__s.end(); + it != e; ++it) { + int __FUNCTION__s_it = (*it).x + 2; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & __FUNCTION__s_elem : __FUNCTION__s) + // CHECK-FIXES-NEXT: int __FUNCTION__s_it = (__FUNCTION__s_elem).x + 2; +} + +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) { + } + // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & Val : Vals) + + // We cannot use the name "Val" in this loop since there is a reference to + // it in the body of the loop. + for (T::iterator it = Vals.begin(), e = Vals.end(); it != e; ++it) { + *it = sizeof(Val); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & Vals_it : Vals) + // CHECK-FIXES-NEXT: Vals_it = sizeof(Val); + + typedef struct Val TD; + 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) { + } + // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & TD : TDs) + + // "TD" cannot be used in this loop since the typedef is being used. + for (U::iterator it = TDs.begin(), e = TDs.end(); it != e; ++it) { + TD V; + V.x = 5; + } + // CHECK-MESSAGES: :[[@LINE-4]]: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; + + using ns::st; + T sts; + for (T::iterator it = sts.begin(), e = sts.end(); it != e; ++it) { + *it = sizeof(st); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & sts_it : sts) + // CHECK-FIXES-NEXT: sts_it = sizeof(st); +} + +} // namespace NamingConflict + +namespace FreeBeginEnd { + +// FIXME: Loop Convert should detect free begin()/end() functions. + +struct MyArray { + unsigned size(); +}; + +template +struct MyContainer { +}; + +int *begin(const MyArray &Arr); +int *end(const MyArray &Arr); + +template +T *begin(const MyContainer &C); +template +T *end(const MyContainer &C); + +// The Loop Convert Transform doesn't detect free functions begin()/end() and +// so fails to transform these cases which it should. +void f() { + MyArray Arr; + for (unsigned i = 0, e = Arr.size(); i < e; ++i) { + } + + MyContainer C; + for (int *I = begin(C), *E = end(C); I != E; ++I) { + } +} + +} // namespace FreeBeginEnd + +namespace Nesting { + +void f() { + const int N = 10; + const int M = 15; + Val Arr[N]; + for (int i = 0; i < N; ++i) { + for (int j = 0; j < N; ++j) { + int k = Arr[i].x + Arr[j].x; + // The repeat is there to allow FileCheck to make sure the two variable + // names aren't the same. + int l = Arr[i].x + Arr[j].x; + } + } + // CHECK-MESSAGES: :[[@LINE-8]]:3: warning: use range-based for loop instead + // CHECK-MESSAGES: :[[@LINE-8]]:5: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : Arr) + // CHECK-FIXES-NEXT: for (auto & Arr_j : Arr) + // CHECK-FIXES-NEXT: int k = elem.x + Arr_j.x; + // CHECK-FIXES-NOT: int l = elem.x + elem.x; + + // The inner loop is also convertible, but doesn't need to be converted + // immediately. FIXME: update this test when that changes. + Val Nest[N][M]; + for (int i = 0; i < N; ++i) { + for (int j = 0; j < M; ++j) { + printf("Got item %d", Nest[i][j].x); + } + } + // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : Nest) + // CHECK-FIXES-NEXT: for (int j = 0; j < M; ++j) + // CHECK-FIXES-NEXT: printf("Got item %d", elem[j].x); + + // Note that the order of M and N are switched for this test. + for (int j = 0; j < M; ++j) { + for (int i = 0; i < N; ++i) { + printf("Got item %d", Nest[i][j].x); + } + } + // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: use range-based for loop instead + // CHECK-FIXES-NOT: for (auto & {{[a-zA-Z_]+}} : Nest[i]) + // CHECK-FIXES: for (int j = 0; j < M; ++j) + // CHECK-FIXES-NEXT: for (auto & elem : Nest) + // CHECK-FIXES-NEXT: printf("Got item %d", elem[j].x); + + // The inner loop is also convertible. + Nested NestT; + for (Nested::iterator I = NestT.begin(), E = NestT.end(); I != E; ++I) { + for (T::iterator TI = (*I).begin(), TE = (*I).end(); TI != TE; ++TI) { + printf("%d", *TI); + } + } + // 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-NEXT: printf("%d", *TI); + + // The inner loop is also convertible. + Nested NestS; + for (Nested::const_iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) { + for (S::const_iterator SI = (*I).begin(), SE = (*I).end(); SI != SE; ++SI) { + printf("%d", *SI); + } + } + // 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-NEXT: printf("%d", *SI); +} + +} // namespace Nesting + +namespace SingleIterator { + +void complexContainer() { + X exes[5]; + int index = 0; + + for (S::iterator i = exes[index].getS().begin(), e = exes[index].getS().end(); i != e; ++i) { + MutableVal k = *i; + MutableVal j = *i; + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : exes[index].getS()) + // CHECK-FIXES-NEXT: MutableVal k = elem; + // CHECK-FIXES-NEXT: MutableVal j = elem; +} + +void f() { + /// begin()/end() - based for loops here: + T t; + for (T::iterator it = t.begin(); it != t.end(); ++it) { + printf("I found %d\n", *it); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : t) + // CHECK-FIXES-NEXT: printf("I found %d\n", elem); + + T *pt; + for (T::iterator it = pt->begin(); it != pt->end(); ++it) { + printf("I found %d\n", *it); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : *pt) + // CHECK-FIXES-NEXT: printf("I found %d\n", elem); + + S s; + for (S::iterator it = s.begin(); it != s.end(); ++it) { + printf("s has value %d\n", (*it).x); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : s) + // CHECK-FIXES-NEXT: printf("s has value %d\n", (elem).x); + + S *ps; + for (S::iterator it = ps->begin(); it != ps->end(); ++it) { + printf("s has value %d\n", (*it).x); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & p : *ps) + // CHECK-FIXES-NEXT: printf("s has value %d\n", (p).x); + + for (S::iterator it = s.begin(); it != s.end(); ++it) { + printf("s has value %d\n", it->x); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : s) + // CHECK-FIXES-NEXT: printf("s has value %d\n", elem.x); + + for (S::iterator it = s.begin(); it != s.end(); ++it) { + it->x = 3; + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : s) + // CHECK-FIXES-NEXT: elem.x = 3; + + for (S::iterator it = s.begin(); it != s.end(); ++it) { + (*it).x = 3; + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : s) + // CHECK-FIXES-NEXT: (elem).x = 3; + + for (S::iterator it = s.begin(); it != s.end(); ++it) { + it->nonConstFun(4, 5); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : s) + // CHECK-FIXES-NEXT: elem.nonConstFun(4, 5); + + U u; + for (U::iterator it = u.begin(); it != u.end(); ++it) { + printf("s has value %d\n", it->x); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : u) + // CHECK-FIXES-NEXT: printf("s has value %d\n", elem.x); + + for (U::iterator it = u.begin(); it != u.end(); ++it) { + printf("s has value %d\n", (*it).x); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : u) + // CHECK-FIXES-NEXT: printf("s has value %d\n", (elem).x); + + U::iterator A; + for (U::iterator i = u.begin(); i != u.end(); ++i) + int k = A->x + i->x; + // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : u) + // CHECK-FIXES-NEXT: int k = A->x + elem.x; + + dependent v; + for (dependent::iterator it = v.begin(); + it != v.end(); ++it) { + 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-NEXT: printf("Fibonacci number is %d\n", elem); + + for (dependent::iterator it(v.begin()); + it != v.end(); ++it) { + 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-NEXT: printf("Fibonacci number is %d\n", elem); + + doublyDependent intmap; + for (doublyDependent::iterator it = intmap.begin(); + it != intmap.end(); ++it) { + printf("intmap[%d] = %d", it->first, it->second); + } + // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (auto & elem : intmap) + // CHECK-FIXES-NEXT: printf("intmap[%d] = %d", elem.first, elem.second); +} + +void different_type() { + // Tests to verify the proper use of auto where the init variable type and the + // initializer type differ or are mostly the same except for const qualifiers. + + // s.begin() returns a type 'iterator' which is just a non-const pointer and + // differs from const_iterator only on the const qualification. + S s; + for (S::const_iterator it = s.begin(); it != s.end(); ++it) { + printf("s has value %d\n", (*it).x); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (const auto & elem : s) + // CHECK-FIXES-NEXT: printf("s has value %d\n", (elem).x); + + S *ps; + for (S::const_iterator it = ps->begin(); it != ps->end(); ++it) { + printf("s has value %d\n", (*it).x); + } + // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead + // CHECK-FIXES: for (const auto & p : *ps) + // CHECK-FIXES-NEXT: printf("s has value %d\n", (p).x); + + // v.begin() returns a user-defined type 'iterator' which, since it's + // different from const_iterator, disqualifies these loops from + // transformation. + dependent v; + for (dependent::const_iterator it = v.begin(); it != v.end(); ++it) { + printf("Fibonacci number is %d\n", *it); + } + + for (dependent::const_iterator it(v.begin()); it != v.end(); ++it) { + printf("Fibonacci number is %d\n", *it); + } +} + +} 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 new file mode 100644 index 000000000000..ffec61f7dbc8 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/modernize-loop-convert-negative.cpp @@ -0,0 +1,459 @@ +// RUN: $(dirname %s)/check_clang_tidy.sh %s modernize-loop-convert %t -- -std=c++11 -I %S/Inputs/modernize-loop-convert +// REQUIRES: shell + +#include "structures.h" + +// CHECK-FIXES-NOT: for ({{.*[^:]:[^:].*}}) + +namespace Negative { + +const int N = 6; +int arr[N] = {1, 2, 3, 4, 5, 6}; +int (*pArr)[N] = &arr; +int sum = 0; + +// Checks for the index start and end: +void indexStartAndEnd() { + for (int i = 0; i < N + 1; ++i) + sum += arr[i]; + + for (int i = 0; i < N - 1; ++i) + sum += arr[i]; + + for (int i = 1; i < N; ++i) + sum += arr[i]; + + for (int i = 1; i < N; ++i) + sum += arr[i]; + + for (int i = 0;; ++i) + sum += (*pArr)[i]; +} + +// Checks for invalid increment steps: +void increment() { + for (int i = 0; i < N; --i) + sum += arr[i]; + + for (int i = 0; i < N; i) + sum += arr[i]; + + for (int i = 0; i < N;) + sum += arr[i]; + + for (int i = 0; i < N; i += 2) + sum++; +} + +// Checks to make sure that the index isn't used outside of the array: +void indexUse() { + for (int i = 0; i < N; ++i) + arr[i] += 1 + i; +} + +// Check for loops that don't mention arrays +void noArray() { + for (int i = 0; i < N; ++i) + sum += i; + + for (int i = 0; i < N; ++i) { + } + + for (int i = 0; i < N; ++i) + ; +} + +// Checks for incorrect loop variables. +void mixedVariables() { + int badIndex; + for (int i = 0; badIndex < N; ++i) + sum += arr[i]; + + for (int i = 0; i < N; ++badIndex) + sum += arr[i]; + + for (int i = 0; badIndex < N; ++badIndex) + sum += arr[i]; + + for (int i = 0; badIndex < N; ++badIndex) + sum += arr[badIndex]; +} + +// Checks for multiple arrays indexed. +void multipleArrays() { + int badArr[N]; + + for (int i = 0; i < N; ++i) + sum += arr[i] + badArr[i]; + + for (int i = 0; i < N; ++i) { + int k = badArr[i]; + sum += arr[i] + k; + } +} + +struct HasArr { + int Arr[N]; + Val ValArr[N]; +}; + +struct HasIndirectArr { + HasArr HA; + void implicitThis() { + for (int i = 0; i < N; ++i) { + printf("%d", HA.Arr[i]); + } + + for (int i = 0; i < N; ++i) { + printf("%d", HA.ValArr[i].x); + } + } + + void explicitThis() { + for (int i = 0; i < N; ++i) { + printf("%d", this->HA.Arr[i]); + } + + for (int i = 0; i < N; ++i) { + printf("%d", this->HA.ValArr[i].x); + } + } +}; +} + +namespace NegativeIterator { + +S s; +T t; +U u; + +struct BadBeginEnd : T { + iterator notBegin(); + iterator notEnd(); +}; + +void notBeginOrEnd() { + BadBeginEnd Bad; + for (T::iterator i = Bad.notBegin(), e = Bad.end(); i != e; ++i) + int k = *i; + + for (T::iterator i = Bad.begin(), e = Bad.notEnd(); i != e; ++i) + int k = *i; +} + +void badLoopShapes() { + for (T::iterator i = t.begin(), e = t.end(), f = e; i != e; ++i) + int k = *i; + + for (T::iterator i = t.begin(), e = t.end(); i != e;) + int k = *i; + + for (T::iterator i = t.begin(), e = t.end();; ++i) + int k = *i; + + T::iterator outsideI; + T::iterator outsideE; + + for (; outsideI != outsideE; ++outsideI) + int k = *outsideI; +} + +void iteratorArrayMix() { + int lower; + const int N = 6; + for (T::iterator i = t.begin(), e = t.end(); lower < N; ++i) + int k = *i; + + for (T::iterator i = t.begin(), e = t.end(); lower < N; ++lower) + int k = *i; +} + +struct ExtraConstructor : T::iterator { + ExtraConstructor(T::iterator, int); + explicit ExtraConstructor(T::iterator); +}; + +void badConstructor() { + for (T::iterator i = ExtraConstructor(t.begin(), 0), e = t.end(); + i != e; ++i) + int k = *i; + for (T::iterator i = ExtraConstructor(t.begin()), e = t.end(); i != e; ++i) + int k = *i; +} + +void iteratorMemberUsed() { + for (T::iterator i = t.begin(), e = t.end(); i != e; ++i) + i.x = *i; + + for (T::iterator i = t.begin(), e = t.end(); i != e; ++i) + int k = i.x + *i; + + for (T::iterator i = t.begin(), e = t.end(); i != e; ++i) + int k = e.x + *i; +} + +void iteratorMethodCalled() { + for (T::iterator i = t.begin(), e = t.end(); i != e; ++i) + i.insert(3); + + for (T::iterator i = t.begin(), e = t.end(); i != e; ++i) + if (i != i) + int k = 3; +} + +void iteratorOperatorCalled() { + for (T::iterator i = t.begin(), e = t.end(); i != e; ++i) + int k = *(++i); + + for (S::iterator i = s.begin(), e = s.end(); i != e; ++i) + MutableVal k = *(++i); +} + +void differentContainers() { + T other; + for (T::iterator i = t.begin(), e = other.end(); i != e; ++i) + int k = *i; + + for (T::iterator i = other.begin(), e = t.end(); i != e; ++i) + int k = *i; + + S otherS; + for (S::iterator i = s.begin(), e = otherS.end(); i != e; ++i) + MutableVal k = *i; + + for (S::iterator i = otherS.begin(), e = s.end(); i != e; ++i) + MutableVal k = *i; +} + +void wrongIterators() { + T::iterator other; + for (T::iterator i = t.begin(), e = t.end(); i != other; ++i) + int k = *i; +} + +struct EvilArrow : U { + // Please, no one ever write code like this. + U *operator->(); +}; + +void differentMemberAccessTypes() { + EvilArrow A; + for (EvilArrow::iterator i = A.begin(), e = A->end(); i != e; ++i) + Val k = *i; + for (EvilArrow::iterator i = A->begin(), e = A.end(); i != e; ++i) + Val k = *i; +} + +void f(const T::iterator &it, int); +void f(const T &it, int); +void g(T &it, int); + +void iteratorPassedToFunction() { + for (T::iterator i = t.begin(), e = t.end(); i != e; ++i) + f(i, *i); +} + +// FIXME: These tests can be removed if this tool ever does enough analysis to +// decide that this is a safe transformation. Until then, we don't want it +// applied. +void iteratorDefinedOutside() { + T::iterator theEnd = t.end(); + for (T::iterator i = t.begin(); i != theEnd; ++i) + int k = *i; + + T::iterator theBegin = t.begin(); + for (T::iterator e = t.end(); theBegin != e; ++theBegin) + int k = *theBegin; +} + +} // namespace NegativeIterator + +namespace NegativePseudoArray { + +const int N = 6; +dependent v; +dependent *pv; + +transparent> cv; +int sum = 0; + +// Checks for the index start and end: +void indexStartAndEnd() { + for (int i = 0; i < v.size() + 1; ++i) + sum += v[i]; + + for (int i = 0; i < v.size() - 1; ++i) + sum += v[i]; + + for (int i = 1; i < v.size(); ++i) + sum += v[i]; + + for (int i = 1; i < v.size(); ++i) + sum += v[i]; + + for (int i = 0;; ++i) + sum += (*pv)[i]; +} + +// Checks for invalid increment steps: +void increment() { + for (int i = 0; i < v.size(); --i) + sum += v[i]; + + for (int i = 0; i < v.size(); i) + sum += v[i]; + + for (int i = 0; i < v.size();) + sum += v[i]; + + for (int i = 0; i < v.size(); i += 2) + sum++; +} + +// Checks to make sure that the index isn't used outside of the container: +void indexUse() { + for (int i = 0; i < v.size(); ++i) + v[i] += 1 + i; +} + +// Checks for incorrect loop variables. +void mixedVariables() { + int badIndex; + for (int i = 0; badIndex < v.size(); ++i) + sum += v[i]; + + for (int i = 0; i < v.size(); ++badIndex) + sum += v[i]; + + for (int i = 0; badIndex < v.size(); ++badIndex) + sum += v[i]; + + for (int i = 0; badIndex < v.size(); ++badIndex) + sum += v[badIndex]; +} + +// Checks for an array indexed in addition to the container. +void multipleArrays() { + int badArr[N]; + + for (int i = 0; i < v.size(); ++i) + sum += v[i] + badArr[i]; + + for (int i = 0; i < v.size(); ++i) + sum += badArr[i]; + + for (int i = 0; i < v.size(); ++i) { + int k = badArr[i]; + sum += k + 2; + } + + for (int i = 0; i < v.size(); ++i) { + int k = badArr[i]; + sum += v[i] + k; + } +} + +// Checks for multiple containers being indexed container. +void multipleContainers() { + dependent badArr; + + for (int i = 0; i < v.size(); ++i) + sum += v[i] + badArr[i]; + + for (int i = 0; i < v.size(); ++i) + sum += badArr[i]; + + for (int i = 0; i < v.size(); ++i) { + int k = badArr[i]; + sum += k + 2; + } + + for (int i = 0; i < v.size(); ++i) { + int k = badArr[i]; + sum += v[i] + k; + } +} + +// Check to make sure that dereferenced pointers-to-containers behave nicely. +void derefContainer() { + // Note the dependent::operator*() returns another dependent. + // This test makes sure that we don't allow an arbitrary number of *'s. + for (int i = 0; i < pv->size(); ++i) + sum += (**pv).at(i); + + for (int i = 0; i < pv->size(); ++i) + sum += (**pv)[i]; +} + +void wrongEnd() { + int bad; + for (int i = 0, e = v.size(); i < bad; ++i) + sum += v[i]; +} + +// Checks to see that non-const member functions are not called on the container +// object. +// These could be conceivably allowed with a lower required confidence level. +void memberFunctionCalled() { + for (int i = 0; i < v.size(); ++i) { + sum += v[i]; + v.foo(); + } + + for (int i = 0; i < v.size(); ++i) { + sum += v[i]; + dependent::iterator it = v.begin(); + } +} + +} // namespace NegativePseudoArray + +namespace NegativeMultiEndCall { + +S s; +T t; +U u; + +void f(X); +void f(S); +void f(T); + +void complexContainer() { + X x; + for (S::iterator i = x.s.begin(), e = x.s.end(); i != e; ++i) { + f(x); + MutableVal k = *i; + } + + for (T::iterator i = x.t.begin(), e = x.t.end(); i != e; ++i) { + f(x); + int k = *i; + } + + for (S::iterator i = x.s.begin(), e = x.s.end(); i != e; ++i) { + f(x.s); + MutableVal k = *i; + } + + for (T::iterator i = x.t.begin(), e = x.t.end(); i != e; ++i) { + f(x.t); + int k = *i; + } + + for (S::iterator i = x.getS().begin(), e = x.getS().end(); i != e; ++i) { + f(x.getS()); + MutableVal k = *i; + } + + X exes[5]; + int index = 0; + + for (S::iterator i = exes[index].getS().begin(), + e = exes[index].getS().end(); + i != e; ++i) { + index++; + MutableVal k = *i; + } +} + +} // namespace NegativeMultiEndCall