angelgarcia created this revision.
angelgarcia added a reviewer: alexfh.
angelgarcia added subscribers: klimek, cfe-commits.
Reduced the amount of wrong conversions of this check.
http://reviews.llvm.org/D12530
Files:
clang-tidy/modernize/LoopConvertCheck.cpp
clang-tidy/modernize/LoopConvertUtils.cpp
test/clang-tidy/modernize-loop-convert-basic.cpp
test/clang-tidy/modernize-loop-convert-extra.cpp
Index: test/clang-tidy/modernize-loop-convert-extra.cpp
===================================================================
--- test/clang-tidy/modernize-loop-convert-extra.cpp
+++ test/clang-tidy/modernize-loop-convert-extra.cpp
@@ -387,6 +387,14 @@
namespace Nesting {
+void g(S::iterator it);
+void const_g(S::const_iterator it);
+class Foo {
+ public:
+ void g(S::iterator it);
+ void const_g(S::const_iterator it);
+};
+
void f() {
const int N = 10;
const int M = 15;
@@ -454,6 +462,48 @@
// CHECK-FIXES: for (const auto & elem : NestS) {
// CHECK-FIXES-NEXT: for (S::const_iterator SI = (elem).begin(), SE = (elem).end(); SI != SE; ++SI) {
// CHECK-FIXES-NEXT: printf("%d", *SI);
+
+ for (Nested<S>::const_iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) {
+ const S &s = *I;
+ for (S::const_iterator SI = s.begin(), SE = s.end(); SI != SE; ++SI) {
+ printf("%d", *SI);
+ const_g(SI);
+ }
+ }
+ // CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead
+ // CHECK-FIXES: for (const auto & s : NestS) {
+
+ for (Nested<S>::iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) {
+ S &s = *I;
+ for (S::iterator SI = s.begin(), SE = s.end(); SI != SE; ++SI) {
+ printf("%d", *SI);
+ g(SI);
+ }
+ }
+ // CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead
+ // CHECK-FIXES: for (auto & s : NestS) {
+
+ Foo foo;
+ for (Nested<S>::const_iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) {
+ const S &s = *I;
+ for (S::const_iterator SI = s.begin(), SE = s.end(); SI != SE; ++SI) {
+ printf("%d", *SI);
+ foo.const_g(SI);
+ }
+ }
+ // CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead
+ // CHECK-FIXES: for (const auto & s : NestS) {
+
+ for (Nested<S>::iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) {
+ S &s = *I;
+ for (S::iterator SI = s.begin(), SE = s.end(); SI != SE; ++SI) {
+ printf("%d", *SI);
+ foo.g(SI);
+ }
+ }
+ // CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead
+ // CHECK-FIXES: for (auto & s : NestS) {
+
}
} // namespace Nesting
Index: test/clang-tidy/modernize-loop-convert-basic.cpp
===================================================================
--- test/clang-tidy/modernize-loop-convert-basic.cpp
+++ test/clang-tidy/modernize-loop-convert-basic.cpp
@@ -427,6 +427,27 @@
}
};
+void foo(S::iterator it) {}
+class Foo {public: void bar(S::iterator it); };
+
+void iterator_used() {
+ S s;
+ Foo fo;
+
+ for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) {
+ foo(it);
+ }
+
+ for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) {
+ fo.bar(it);
+ }
+
+ S::iterator ret;
+ for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) {
+ ret = it;
+ }
+}
+
} // namespace Iterator
namespace PseudoArray {
@@ -494,12 +515,12 @@
for (auto i = 0; i < v.size(); ++i) {
}
// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
- // CHECK-FIXES: for (auto & elem : v) {
+ // CHECK-FIXES: for (const auto & elem : v) {
for (auto i = 0; i < v.size(); ++i)
;
// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
- // CHECK-FIXES: for (auto & elem : v)
+ // CHECK-FIXES: for (const auto & elem : v)
}
struct NoBeginEnd {
@@ -509,15 +530,15 @@
struct NoConstBeginEnd {
NoConstBeginEnd();
unsigned size() const;
- unsigned begin();
- unsigned end();
+ unsigned* begin();
+ unsigned* end();
};
struct ConstBeginEnd {
ConstBeginEnd();
unsigned size() const;
- unsigned begin() const;
- unsigned end() const;
+ unsigned* begin() const;
+ unsigned* end() const;
};
// Shouldn't transform pseudo-array uses if the container doesn't provide
@@ -535,13 +556,32 @@
for (unsigned i = 0, e = CBE.size(); i < e; ++i) {
}
// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
- // CHECK-FIXES: for (auto & elem : CBE) {
+ // CHECK-FIXES: for (const auto & elem : CBE) {
const ConstBeginEnd const_CBE;
for (unsigned i = 0, e = const_CBE.size(); i < e; ++i) {
}
// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
- // CHECK-FIXES: for (auto & elem : const_CBE) {
+ // CHECK-FIXES: for (const auto & elem : const_CBE) {
+}
+
+struct DerefByValue {
+ DerefByValue();
+ struct iter { unsigned operator*(); };
+ unsigned size() const;
+ iter begin();
+ iter end();
+ unsigned operator[](int);
+};
+
+void DerefByValueTest() {
+ DerefByValue DBV;
+ for (unsigned i = 0, e = DBV.size(); i < e; ++i) {
+ printf("%d\n", DBV[i]);
+ }
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+ // CHECK-FIXES: for (auto && elem : DBV) {
+
}
} // namespace PseudoArray
Index: clang-tidy/modernize/LoopConvertUtils.cpp
===================================================================
--- clang-tidy/modernize/LoopConvertUtils.cpp
+++ clang-tidy/modernize/LoopConvertUtils.cpp
@@ -425,12 +425,8 @@
ConfidenceLevel(Confidence::CL_Safe), NextStmtParent(nullptr),
CurrStmtParent(nullptr), ReplaceWithAliasUse(false),
AliasFromForInit(false) {
- if (ContainerExpr) {
+ if (ContainerExpr)
addComponent(ContainerExpr);
- FoldingSetNodeID ID;
- const Expr *E = ContainerExpr->IgnoreParenImpCasts();
- E->Profile(ID, *Context, true);
- }
}
bool ForLoopIndexUseVisitor::findAndVerifyUsages(const Stmt *Body) {
@@ -470,7 +466,7 @@
return true;
}
- return VisitorBase::TraverseUnaryOperator(Uop);
+ return VisitorBase::TraverseUnaryDeref(Uop);
}
/// \brief If the member expression is operator-> (overloaded or not) on
@@ -521,7 +517,13 @@
}
}
- if (Member->isArrow() && Obj && exprReferencesVariable(IndexVar, Obj)) {
+ if (Obj && exprReferencesVariable(IndexVar, Obj)) {
+ // Member calls on the iterator with '.' are not allowed.
+ if (!Member->isArrow()) {
+ OnlyUsedAsIndex = false;
+ return true;
+ }
+
if (ExprType.isNull())
ExprType = Obj->getType();
@@ -539,7 +541,7 @@
return true;
}
}
- return TraverseStmt(Member->getBase());
+ return VisitorBase::TraverseMemberExpr(Member);
}
/// \brief If a member function call is the at() accessor on the container with
@@ -576,7 +578,7 @@
}
/// \brief If an overloaded operator call is a dereference of IndexVar or
-/// a subscript of a the container with IndexVar as the single argument,
+/// a subscript of the container with IndexVar as the single argument,
/// include it as a valid usage and prune the traversal.
///
/// For example, given
@@ -682,9 +684,6 @@
/// i.insert(0);
/// for (vector<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,8 @@
/// \endcode
bool ForLoopIndexUseVisitor::VisitDeclRefExpr(DeclRefExpr *E) {
const ValueDecl *TheDecl = E->getDecl();
- if (areSameVariable(IndexVar, TheDecl) || areSameVariable(EndVar, TheDecl))
+ if (areSameVariable(IndexVar, TheDecl) || exprReferencesVariable(IndexVar, E)
+ || areSameVariable(EndVar, TheDecl) || exprReferencesVariable(EndVar, E))
OnlyUsedAsIndex = false;
if (containsExpr(Context, &DependentExprs, E))
ConfidenceLevel.lowerTo(Confidence::CL_Risky);
Index: clang-tidy/modernize/LoopConvertCheck.cpp
===================================================================
--- clang-tidy/modernize/LoopConvertCheck.cpp
+++ clang-tidy/modernize/LoopConvertCheck.cpp
@@ -364,6 +364,23 @@
return false;
}
+/// \brief Returns true when it can be guaranteed that the elements of the
+/// container are not being modified.
+static bool usagesAreConst(const UsageResult &Usages) {
+ // FIXME: Make this function more generic.
+ return Usages.empty();
+}
+
+/// \brief Returns true if the elements of the container are never accessed
+/// by reference.
+static bool usagesReturnRValues(const UsageResult &Usages) {
+ for (const auto &U : Usages) {
+ if (!U.E->isRValue())
+ return false;
+ }
+ return true;
+}
+
LoopConvertCheck::LoopConvertCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context), TUInfo(new TUTrackingInfo),
MinConfidence(StringSwitch<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);
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
