Fix several corner cases for loop-convert check.
Summary: Reduced the amount of wrong conversions of this check.
Reviewers: klimek
Subscribers: alexfh, cfe-commits
Differential Revision: http://reviews.llvm.org/D12530
llvm-svn: 246550
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index 11d5425..b6cb64f 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -364,6 +364,23 @@
return false;
}
+/// \brief Returns true when it can be guaranteed that the elements of the
+/// container are not being modified.
+static bool usagesAreConst(const UsageResult &Usages) {
+ // FIXME: Make this function more generic.
+ return Usages.empty();
+}
+
+/// \brief Returns true if the elements of the container are never accessed
+/// by reference.
+static bool usagesReturnRValues(const UsageResult &Usages) {
+ for (const auto &U : Usages) {
+ if (!U.Expression->isRValue())
+ return false;
+ }
+ return true;
+}
+
LoopConvertCheck::LoopConvertCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context), TUInfo(new TUTrackingInfo),
MinConfidence(StringSwitch<Confidence::Level>(
@@ -452,7 +469,8 @@
StringRef MaybeDereference = ContainerNeedsDereference ? "*" : "";
std::string TypeString = AutoRefType.getAsString();
std::string Range = ("(" + TypeString + " " + VarName + " : " +
- MaybeDereference + ContainerString + ")").str();
+ MaybeDereference + ContainerString + ")")
+ .str();
Diag << FixItHint::CreateReplacement(
CharSourceRange::getTokenRange(ParenRange), Range);
TUInfo->getGeneratedDecls().insert(make_pair(TheLoop, VarName));
@@ -464,7 +482,7 @@
StringRef LoopConvertCheck::checkRejections(ASTContext *Context,
const Expr *ContainerExpr,
const ForStmt *TheLoop) {
- // If we already modified the reange of this for loop, don't do any further
+ // If we already modified the range of this for loop, don't do any further
// updates on this iteration.
if (TUInfo->getReplacedVars().count(TheLoop))
return "";
@@ -525,6 +543,18 @@
if (!getReferencedVariable(ContainerExpr) &&
!isDirectMemberExpr(ContainerExpr))
ConfidenceLevel.lowerTo(Confidence::CL_Risky);
+ } else if (FixerKind == LFK_PseudoArray) {
+ if (!DerefByValue && !DerefByConstRef) {
+ const UsageResult &Usages = Finder.getUsages();
+ if (usagesAreConst(Usages)) {
+ // FIXME: check if the type is trivially copiable.
+ DerefByConstRef = true;
+ } else if (usagesReturnRValues(Usages)) {
+ // If the index usages (dereference, subscript, at) return RValues,
+ // then we should not use a non-const reference.
+ DerefByValue = true;
+ }
+ }
}
StringRef ContainerString = checkRejections(Context, ContainerExpr, TheLoop);
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
index ace0aee..732d40b 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
@@ -425,12 +425,8 @@
ConfidenceLevel(Confidence::CL_Safe), NextStmtParent(nullptr),
CurrStmtParent(nullptr), ReplaceWithAliasUse(false),
AliasFromForInit(false) {
- if (ContainerExpr) {
+ if (ContainerExpr)
addComponent(ContainerExpr);
- FoldingSetNodeID ID;
- const Expr *E = ContainerExpr->IgnoreParenImpCasts();
- E->Profile(ID, *Context, true);
- }
}
bool ForLoopIndexUseVisitor::findAndVerifyUsages(const Stmt *Body) {
@@ -521,7 +517,13 @@
}
}
- if (Member->isArrow() && Obj && exprReferencesVariable(IndexVar, Obj)) {
+ if (Obj && exprReferencesVariable(IndexVar, Obj)) {
+ // Member calls on the iterator with '.' are not allowed.
+ if (!Member->isArrow()) {
+ OnlyUsedAsIndex = false;
+ return true;
+ }
+
if (ExprType.isNull())
ExprType = Obj->getType();
@@ -539,7 +541,7 @@
return true;
}
}
- return TraverseStmt(Member->getBase());
+ return VisitorBase::TraverseMemberExpr(Member);
}
/// \brief If a member function call is the at() accessor on the container with
@@ -576,7 +578,7 @@
}
/// \brief If an overloaded operator call is a dereference of IndexVar or
-/// a subscript of a the container with IndexVar as the single argument,
+/// a subscript of the container with IndexVar as the single argument,
/// include it as a valid usage and prune the traversal.
///
/// For example, given
@@ -682,9 +684,6 @@
/// i.insert(0);
/// for (vector<int>::iterator i = container.begin(), e = container.end();
/// i != e; ++i)
-/// i.insert(0);
-/// for (vector<int>::iterator i = container.begin(), e = container.end();
-/// i != e; ++i)
/// if (i + 1 != e)
/// printf("%d", *i);
/// \endcode
@@ -700,7 +699,9 @@
/// \endcode
bool ForLoopIndexUseVisitor::VisitDeclRefExpr(DeclRefExpr *E) {
const ValueDecl *TheDecl = E->getDecl();
- if (areSameVariable(IndexVar, TheDecl) || areSameVariable(EndVar, TheDecl))
+ if (areSameVariable(IndexVar, TheDecl) ||
+ exprReferencesVariable(IndexVar, E) || areSameVariable(EndVar, TheDecl) ||
+ exprReferencesVariable(EndVar, E))
OnlyUsedAsIndex = false;
if (containsExpr(Context, &DependentExprs, E))
ConfidenceLevel.lowerTo(Confidence::CL_Risky);
diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
index 47da665..ec29b67 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
@@ -197,14 +197,14 @@
/// \brief The information needed to describe a valid convertible usage
/// of an array index or iterator.
struct Usage {
- const Expr *E;
+ const Expr *Expression;
bool IsArrow;
SourceRange Range;
explicit Usage(const Expr *E)
- : E(E), IsArrow(false), Range(E->getSourceRange()) {}
+ : Expression(E), IsArrow(false), Range(Expression->getSourceRange()) {}
Usage(const Expr *E, bool IsArrow, SourceRange Range)
- : E(E), IsArrow(IsArrow), Range(std::move(Range)) {}
+ : Expression(E), IsArrow(IsArrow), Range(std::move(Range)) {}
};
/// \brief A class to encapsulate lowering of the tool's confidence level.