Skip to content

Commit 692cbb5

Browse files
angargoangargo
committed
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
1 parent 7a9495b commit 692cbb5

File tree

6 files changed

+334
-217
lines changed

6 files changed

+334
-217
lines changed

‎clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,23 @@ static bool isDirectMemberExpr(const Expr *E) {
364364
return false;
365365
}
366366

367+
/// \brief Returns true when it can be guaranteed that the elements of the
368+
/// container are not being modified.
369+
static bool usagesAreConst(const UsageResult &Usages) {
370+
// FIXME: Make this function more generic.
371+
return Usages.empty();
372+
}
373+
374+
/// \brief Returns true if the elements of the container are never accessed
375+
/// by reference.
376+
static bool usagesReturnRValues(const UsageResult &Usages) {
377+
for (const auto &U : Usages) {
378+
if (!U.Expression->isRValue())
379+
return false;
380+
}
381+
return true;
382+
}
383+
367384
LoopConvertCheck::LoopConvertCheck(StringRef Name, ClangTidyContext *Context)
368385
: ClangTidyCheck(Name, Context), TUInfo(new TUTrackingInfo),
369386
MinConfidence(StringSwitch<Confidence::Level>(
@@ -452,7 +469,8 @@ void LoopConvertCheck::doConversion(
452469
StringRef MaybeDereference = ContainerNeedsDereference ? "*" : "";
453470
std::string TypeString = AutoRefType.getAsString();
454471
std::string Range = ("(" + TypeString + " " + VarName + " : " +
455-
MaybeDereference + ContainerString + ")").str();
472+
MaybeDereference + ContainerString + ")")
473+
.str();
456474
Diag << FixItHint::CreateReplacement(
457475
CharSourceRange::getTokenRange(ParenRange), Range);
458476
TUInfo->getGeneratedDecls().insert(make_pair(TheLoop, VarName));
@@ -464,7 +482,7 @@ void LoopConvertCheck::doConversion(
464482
StringRef LoopConvertCheck::checkRejections(ASTContext *Context,
465483
const Expr *ContainerExpr,
466484
const ForStmt *TheLoop) {
467-
// If we already modified the reange of this for loop, don't do any further
485+
// If we already modified the range of this for loop, don't do any further
468486
// updates on this iteration.
469487
if (TUInfo->getReplacedVars().count(TheLoop))
470488
return "";
@@ -525,6 +543,18 @@ void LoopConvertCheck::findAndVerifyUsages(
525543
if (!getReferencedVariable(ContainerExpr) &&
526544
!isDirectMemberExpr(ContainerExpr))
527545
ConfidenceLevel.lowerTo(Confidence::CL_Risky);
546+
} else if (FixerKind == LFK_PseudoArray) {
547+
if (!DerefByValue && !DerefByConstRef) {
548+
const UsageResult &Usages = Finder.getUsages();
549+
if (usagesAreConst(Usages)) {
550+
// FIXME: check if the type is trivially copiable.
551+
DerefByConstRef = true;
552+
} else if (usagesReturnRValues(Usages)) {
553+
// If the index usages (dereference, subscript, at) return RValues,
554+
// then we should not use a non-const reference.
555+
DerefByValue = true;
556+
}
557+
}
528558
}
529559

530560
StringRef ContainerString = checkRejections(Context, ContainerExpr, TheLoop);

‎clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -425,12 +425,8 @@ ForLoopIndexUseVisitor::ForLoopIndexUseVisitor(ASTContext *Context,
425425
ConfidenceLevel(Confidence::CL_Safe), NextStmtParent(nullptr),
426426
CurrStmtParent(nullptr), ReplaceWithAliasUse(false),
427427
AliasFromForInit(false) {
428-
if (ContainerExpr) {
428+
if (ContainerExpr)
429429
addComponent(ContainerExpr);
430-
FoldingSetNodeID ID;
431-
const Expr *E = ContainerExpr->IgnoreParenImpCasts();
432-
E->Profile(ID, *Context, true);
433-
}
434430
}
435431

436432
bool ForLoopIndexUseVisitor::findAndVerifyUsages(const Stmt *Body) {
@@ -521,7 +517,13 @@ bool ForLoopIndexUseVisitor::TraverseMemberExpr(MemberExpr *Member) {
521517
}
522518
}
523519

524-
if (Member->isArrow() && Obj && exprReferencesVariable(IndexVar, Obj)) {
520+
if (Obj && exprReferencesVariable(IndexVar, Obj)) {
521+
// Member calls on the iterator with '.' are not allowed.
522+
if (!Member->isArrow()) {
523+
OnlyUsedAsIndex = false;
524+
return true;
525+
}
526+
525527
if (ExprType.isNull())
526528
ExprType = Obj->getType();
527529

@@ -539,7 +541,7 @@ bool ForLoopIndexUseVisitor::TraverseMemberExpr(MemberExpr *Member) {
539541
return true;
540542
}
541543
}
542-
return TraverseStmt(Member->getBase());
544+
return VisitorBase::TraverseMemberExpr(Member);
543545
}
544546

545547
/// \brief If a member function call is the at() accessor on the container with
@@ -576,7 +578,7 @@ bool ForLoopIndexUseVisitor::TraverseCXXMemberCallExpr(
576578
}
577579

578580
/// \brief If an overloaded operator call is a dereference of IndexVar or
579-
/// a subscript of a the container with IndexVar as the single argument,
581+
/// a subscript of the container with IndexVar as the single argument,
580582
/// include it as a valid usage and prune the traversal.
581583
///
582584
/// For example, given
@@ -682,9 +684,6 @@ bool ForLoopIndexUseVisitor::TraverseArraySubscriptExpr(ArraySubscriptExpr *E) {
682684
/// i.insert(0);
683685
/// for (vector<int>::iterator i = container.begin(), e = container.end();
684686
/// i != e; ++i)
685-
/// i.insert(0);
686-
/// for (vector<int>::iterator i = container.begin(), e = container.end();
687-
/// i != e; ++i)
688687
/// if (i + 1 != e)
689688
/// printf("%d", *i);
690689
/// \endcode
@@ -700,7 +699,9 @@ bool ForLoopIndexUseVisitor::TraverseArraySubscriptExpr(ArraySubscriptExpr *E) {
700699
/// \endcode
701700
bool ForLoopIndexUseVisitor::VisitDeclRefExpr(DeclRefExpr *E) {
702701
const ValueDecl *TheDecl = E->getDecl();
703-
if (areSameVariable(IndexVar, TheDecl) || areSameVariable(EndVar, TheDecl))
702+
if (areSameVariable(IndexVar, TheDecl) ||
703+
exprReferencesVariable(IndexVar, E) || areSameVariable(EndVar, TheDecl) ||
704+
exprReferencesVariable(EndVar, E))
704705
OnlyUsedAsIndex = false;
705706
if (containsExpr(Context, &DependentExprs, E))
706707
ConfidenceLevel.lowerTo(Confidence::CL_Risky);

‎clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -197,14 +197,14 @@ class DeclFinderASTVisitor
197197
/// \brief The information needed to describe a valid convertible usage
198198
/// of an array index or iterator.
199199
struct Usage {
200-
const Expr *E;
200+
const Expr *Expression;
201201
bool IsArrow;
202202
SourceRange Range;
203203

204204
explicit Usage(const Expr *E)
205-
: E(E), IsArrow(false), Range(E->getSourceRange()) {}
205+
: Expression(E), IsArrow(false), Range(Expression->getSourceRange()) {}
206206
Usage(const Expr *E, bool IsArrow, SourceRange Range)
207-
: E(E), IsArrow(IsArrow), Range(std::move(Range)) {}
207+
: Expression(E), IsArrow(IsArrow), Range(std::move(Range)) {}
208208
};
209209

210210
/// \brief A class to encapsulate lowering of the tool's confidence level.

‎clang-tools-extra/test/clang-tidy/modernize-loop-convert-basic.cpp

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -494,12 +494,12 @@ void noContainer() {
494494
for (auto i = 0; i < v.size(); ++i) {
495495
}
496496
// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
497-
// CHECK-FIXES: for (auto & elem : v) {
497+
// CHECK-FIXES: for (const auto & elem : v) {
498498

499499
for (auto i = 0; i < v.size(); ++i)
500500
;
501501
// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
502-
// CHECK-FIXES: for (auto & elem : v)
502+
// CHECK-FIXES: for (const auto & elem : v)
503503
}
504504

505505
struct NoBeginEnd {
@@ -509,15 +509,15 @@ struct NoBeginEnd {
509509
struct NoConstBeginEnd {
510510
NoConstBeginEnd();
511511
unsigned size() const;
512-
unsigned begin();
513-
unsigned end();
512+
unsigned* begin();
513+
unsigned* end();
514514
};
515515

516516
struct ConstBeginEnd {
517517
ConstBeginEnd();
518518
unsigned size() const;
519-
unsigned begin() const;
520-
unsigned end() const;
519+
unsigned* begin() const;
520+
unsigned* end() const;
521521
};
522522

523523
// Shouldn't transform pseudo-array uses if the container doesn't provide
@@ -535,13 +535,32 @@ void NoBeginEndTest() {
535535
for (unsigned i = 0, e = CBE.size(); i < e; ++i) {
536536
}
537537
// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
538-
// CHECK-FIXES: for (auto & elem : CBE) {
538+
// CHECK-FIXES: for (const auto & elem : CBE) {
539539

540540
const ConstBeginEnd const_CBE;
541541
for (unsigned i = 0, e = const_CBE.size(); i < e; ++i) {
542542
}
543543
// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
544-
// CHECK-FIXES: for (auto & elem : const_CBE) {
544+
// CHECK-FIXES: for (const auto & elem : const_CBE) {
545+
}
546+
547+
struct DerefByValue {
548+
DerefByValue();
549+
struct iter { unsigned operator*(); };
550+
unsigned size() const;
551+
iter begin();
552+
iter end();
553+
unsigned operator[](int);
554+
};
555+
556+
void DerefByValueTest() {
557+
DerefByValue DBV;
558+
for (unsigned i = 0, e = DBV.size(); i < e; ++i) {
559+
printf("%d\n", DBV[i]);
560+
}
561+
// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
562+
// CHECK-FIXES: for (auto && elem : DBV) {
563+
545564
}
546565

547566
} // namespace PseudoArray

‎clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -387,6 +387,14 @@ void f() {
387387

388388
namespace Nesting {
389389

390+
void g(S::iterator it);
391+
void const_g(S::const_iterator it);
392+
class Foo {
393+
public:
394+
void g(S::iterator it);
395+
void const_g(S::const_iterator it);
396+
};
397+
390398
void f() {
391399
const int N = 10;
392400
const int M = 15;
@@ -454,6 +462,48 @@ void f() {
454462
// CHECK-FIXES: for (const auto & elem : NestS) {
455463
// CHECK-FIXES-NEXT: for (S::const_iterator SI = (elem).begin(), SE = (elem).end(); SI != SE; ++SI) {
456464
// CHECK-FIXES-NEXT: printf("%d", *SI);
465+
466+
for (Nested<S>::const_iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) {
467+
const S &s = *I;
468+
for (S::const_iterator SI = s.begin(), SE = s.end(); SI != SE; ++SI) {
469+
printf("%d", *SI);
470+
const_g(SI);
471+
}
472+
}
473+
// CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead
474+
// CHECK-FIXES: for (const auto & s : NestS) {
475+
476+
for (Nested<S>::iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) {
477+
S &s = *I;
478+
for (S::iterator SI = s.begin(), SE = s.end(); SI != SE; ++SI) {
479+
printf("%d", *SI);
480+
g(SI);
481+
}
482+
}
483+
// CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead
484+
// CHECK-FIXES: for (auto & s : NestS) {
485+
486+
Foo foo;
487+
for (Nested<S>::const_iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) {
488+
const S &s = *I;
489+
for (S::const_iterator SI = s.begin(), SE = s.end(); SI != SE; ++SI) {
490+
printf("%d", *SI);
491+
foo.const_g(SI);
492+
}
493+
}
494+
// CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead
495+
// CHECK-FIXES: for (const auto & s : NestS) {
496+
497+
for (Nested<S>::iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) {
498+
S &s = *I;
499+
for (S::iterator SI = s.begin(), SE = s.end(); SI != SE; ++SI) {
500+
printf("%d", *SI);
501+
foo.g(SI);
502+
}
503+
}
504+
// CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead
505+
// CHECK-FIXES: for (auto & s : NestS) {
506+
457507
}
458508

459509
} // namespace Nesting

0 commit comments

Comments
 (0)