diff options
Diffstat (limited to 'clang-tools-extra/clang-tidy')
4 files changed, 80 insertions, 12 deletions
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp index 9af7979..73d93e3 100644 --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp @@ -381,6 +381,20 @@ static bool usagesReturnRValues(const UsageResult &Usages) { return true; } +/// \brief Returns true if the container is const-qualified. +static bool containerIsConst(const Expr *ContainerExpr, bool Dereference) { + if (const auto *VDec = getReferencedVariable(ContainerExpr)) { + QualType CType = VDec->getType(); + if (Dereference) { + if (!CType->isPointerType()) + return false; + CType = CType->getPointeeType(); + } + return CType.isConstQualified(); + } + return false; +} + LoopConvertCheck::LoopConvertCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), TUInfo(new TUTrackingInfo), MinConfidence(StringSwitch<Confidence::Level>( @@ -401,6 +415,11 @@ void LoopConvertCheck::doConversion( StringRef ContainerString, const UsageResult &Usages, const DeclStmt *AliasDecl, bool AliasUseRequired, bool AliasFromForInit, const ForStmt *TheLoop, RangeDescriptor Descriptor) { + // If there aren't any usages, converting the loop would generate an unused + // variable warning. + if (Usages.size() == 0) + return; + auto Diag = diag(TheLoop->getForLoc(), "use range-based for loop instead"); std::string VarName; @@ -436,11 +455,23 @@ void LoopConvertCheck::doConversion( VarName = Namer.createIndexName(); // First, replace all usages of the array subscript expression with our new // variable. - for (const auto &I : Usages) { - std::string ReplaceText = I.IsArrow ? VarName + "." : VarName; + for (const auto &Usage : Usages) { + std::string ReplaceText; + if (Usage.Expression) { + // If this is an access to a member through the arrow operator, after + // the replacement it must be accessed through the '.' operator. + ReplaceText = Usage.Kind == Usage::UK_MemberThroughArrow ? VarName + "." + : VarName; + } else { + // The Usage expression is only null in case of lambda captures (which + // are VarDecl). If the index is captured by value, add '&' to capture + // by reference instead. + ReplaceText = + Usage.Kind == Usage::UK_CaptureByCopy ? "&" + VarName : VarName; + } TUInfo->getReplacedVars().insert(std::make_pair(TheLoop, IndexVar)); Diag << FixItHint::CreateReplacement( - CharSourceRange::getTokenRange(I.Range), ReplaceText); + CharSourceRange::getTokenRange(Usage.Range), ReplaceText); } } @@ -537,16 +568,24 @@ void LoopConvertCheck::findAndVerifyUsages( if (FixerKind == LFK_Array) { // The array being indexed by IndexVar was discovered during traversal. ContainerExpr = Finder.getContainerIndexed()->IgnoreParenImpCasts(); + // Very few loops are over expressions that generate arrays rather than // array variables. Consider loops over arrays that aren't just represented // by a variable to be risky conversions. if (!getReferencedVariable(ContainerExpr) && !isDirectMemberExpr(ContainerExpr)) ConfidenceLevel.lowerTo(Confidence::CL_Risky); + + // Use 'const' if the array is const. + if (containerIsConst(ContainerExpr, Descriptor.ContainerNeedsDereference)) + Descriptor.DerefByConstRef = true; + } else if (FixerKind == LFK_PseudoArray) { if (!Descriptor.DerefByValue && !Descriptor.DerefByConstRef) { const UsageResult &Usages = Finder.getUsages(); - if (usagesAreConst(Usages)) { + if (usagesAreConst(Usages) || + containerIsConst(ContainerExpr, + Descriptor.ContainerNeedsDereference)) { Descriptor.DerefByConstRef = true; } else if (usagesReturnRValues(Usages)) { // If the index usages (dereference, subscript, at) return RValues, @@ -558,7 +597,7 @@ void LoopConvertCheck::findAndVerifyUsages( if (!U.Expression || U.Expression->getType().isNull()) continue; QualType Type = U.Expression->getType().getCanonicalType(); - if (U.IsArrow) { + if (U.Kind == Usage::UK_MemberThroughArrow) { if (!Type->isPointerType()) continue; Type = Type->getPointeeType(); @@ -625,6 +664,7 @@ void LoopConvertCheck::check(const MatchFinder::MatchResult &Result) { // expression the loop variable is being tested against instead. const auto *EndCall = Nodes.getStmtAs<CXXMemberCallExpr>(EndCallName); const auto *BoundExpr = Nodes.getStmtAs<Expr>(ConditionBoundName); + // If the loop calls end()/size() after each iteration, lower our confidence // level. if (FixerKind != LFK_Array && !EndVar) diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h index 6773b19..d349fdb 100644 --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h @@ -27,9 +27,9 @@ public: private: struct RangeDescriptor { bool ContainerNeedsDereference; + bool DerefByConstRef; bool DerefByValue; bool IsTriviallyCopyable; - bool DerefByConstRef; }; void doConversion(ASTContext *Context, const VarDecl *IndexVar, diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp index d0b2e81..d12d845 100644 --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp @@ -560,7 +560,7 @@ bool ForLoopIndexUseVisitor::TraverseMemberExpr(MemberExpr *Member) { // If something complicated is happening (i.e. the next token isn't an // arrow), give up on making this work. if (!ArrowLoc.isInvalid()) { - addUsage(Usage(ResultExpr, /*IsArrow=*/true, + addUsage(Usage(ResultExpr, Usage::UK_MemberThroughArrow, SourceRange(Base->getExprLoc(), ArrowLoc))); return true; } @@ -762,7 +762,10 @@ bool ForLoopIndexUseVisitor::TraverseLambdaCapture(LambdaExpr *LE, // FIXME: if the index is captured, it will count as an usage and the // alias (if any) won't work, because it is only used in case of having // exactly one usage. - addUsage(Usage(nullptr, false, C->getLocation())); + addUsage(Usage(nullptr, + C->getCaptureKind() == LCK_ByCopy ? Usage::UK_CaptureByCopy + : Usage::UK_CaptureByRef, + C->getLocation())); } } return VisitorBase::TraverseLambdaCapture(LE, C); diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h index 2fda54e..5290b47 100644 --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h @@ -197,14 +197,39 @@ private: /// \brief The information needed to describe a valid convertible usage /// of an array index or iterator. struct Usage { + enum UsageKind { + // Regular usages of the loop index (the ones not specified below). Some + // examples: + // \code + // int X = 8 * Arr[i]; + // ^~~~~~ + // f(param1, param2, *It); + // ^~~ + // if (Vec[i].SomeBool) {} + // ^~~~~~ + // \endcode + UK_Default, + // Indicates whether this is an access to a member through the arrow + // operator on pointers or iterators. + UK_MemberThroughArrow, + // If the variable is being captured by a lambda, indicates whether the + // capture was done by value or by reference. + UK_CaptureByCopy, + UK_CaptureByRef + }; + // The expression that is going to be converted. Null in case of lambda + // captures. const Expr *Expression; - bool IsArrow; + + UsageKind Kind; + + // Range that covers this usage. SourceRange Range; explicit Usage(const Expr *E) - : Expression(E), IsArrow(false), Range(Expression->getSourceRange()) {} - Usage(const Expr *E, bool IsArrow, SourceRange Range) - : Expression(E), IsArrow(IsArrow), Range(std::move(Range)) {} + : Expression(E), Kind(UK_Default), Range(Expression->getSourceRange()) {} + Usage(const Expr *E, UsageKind Kind, SourceRange Range) + : Expression(E), Kind(Kind), Range(std::move(Range)) {} }; /// \brief A class to encapsulate lowering of the tool's confidence level. |