Avoid repeated replacements on loop-convert check.

Summary: The InitListExpr subtree is visited twice, this caused the check to do multiple replacements. Added a set to avoid it.

Reviewers: klimek, alexfh

Subscribers: cfe-commits, alexfh

Differential Revision: http://reviews.llvm.org/D12631

llvm-svn: 246879

3 files changed, 51 insertions, 9 deletions

diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
index a690ecd..116ec5c 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
@@ -460,6 +460,15 @@ void ForLoopIndexUseVisitor::addComponent(const Expr *E) {
   DependentExprs.push_back(std::make_pair(Node, ID));
 }
 
+void ForLoopIndexUseVisitor::addUsage(const Usage &U) {
+  SourceLocation Begin = U.Range.getBegin();
+  if (Begin.isMacroID())
+    Begin = Context->getSourceManager().getSpellingLoc(Begin);
+
+  if (UsageLocations.insert(Begin).second)
+    Usages.push_back(U);
+}
+
 /// \brief If the unary operator is a dereference of IndexVar, include it
 /// as a valid usage and prune the traversal.
 ///
@@ -475,7 +484,7 @@ 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));
+    addUsage(Usage(Uop));
     return true;
   }
 
@@ -549,8 +558,8 @@ 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()) {
-      Usages.push_back(Usage(ResultExpr, /*IsArrow=*/true,
-                             SourceRange(Base->getExprLoc(), ArrowLoc)));
+      addUsage(Usage(ResultExpr, /*IsArrow=*/true,
+                     SourceRange(Base->getExprLoc(), ArrowLoc)));
       return true;
     }
   }
@@ -579,7 +588,7 @@ bool ForLoopIndexUseVisitor::TraverseCXXMemberCallExpr(
     if (isIndexInSubscriptExpr(Context, MemberCall->getArg(0), IndexVar,
                                Member->getBase(), ContainerExpr,
                                ContainerNeedsDereference)) {
-      Usages.push_back(Usage(MemberCall));
+      addUsage(Usage(MemberCall));
       return true;
     }
   }
@@ -614,7 +623,7 @@ bool ForLoopIndexUseVisitor::TraverseCXXOperatorCallExpr(
   switch (OpCall->getOperator()) {
   case OO_Star:
     if (isDereferenceOfOpCall(OpCall, IndexVar)) {
-      Usages.push_back(Usage(OpCall));
+      addUsage(Usage(OpCall));
       return true;
     }
     break;
@@ -625,7 +634,7 @@ bool ForLoopIndexUseVisitor::TraverseCXXOperatorCallExpr(
     if (isIndexInSubscriptExpr(Context, OpCall->getArg(1), IndexVar,
                                OpCall->getArg(0), ContainerExpr,
                                ContainerNeedsDereference)) {
-      Usages.push_back(Usage(OpCall));
+      addUsage(Usage(OpCall));
       return true;
     }
     break;
@@ -674,7 +683,7 @@ bool ForLoopIndexUseVisitor::TraverseArraySubscriptExpr(ArraySubscriptExpr *E) {
   if (!ContainerExpr)
     ContainerExpr = Arr;
 
-  Usages.push_back(Usage(E));
+  addUsage(Usage(E));
   return true;
 }
 
@@ -746,12 +755,12 @@ bool ForLoopIndexUseVisitor::VisitDeclRefExpr(DeclRefExpr *E) {
 bool ForLoopIndexUseVisitor::TraverseLambdaCapture(LambdaExpr *LE,
                                                    const LambdaCapture *C) {
   if (C->capturesVariable()) {
-    const VarDecl* VDecl = C->getCapturedVar();
+    const VarDecl *VDecl = C->getCapturedVar();
     if (areSameVariable(IndexVar, cast<ValueDecl>(VDecl))) {
       // 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.
-      Usages.push_back(Usage(nullptr, false, C->getLocation()));
+      addUsage(Usage(nullptr, false, 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 8a789fa..2fda54e 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
@@ -277,6 +277,9 @@ public:
   /// \brief Accessor for Usages.
   const UsageResult &getUsages() const { return Usages; }
 
+  /// \brief Adds the Usage if it was not added before.
+  void addUsage(const Usage &U);
+
   /// \brief Get the container indexed by IndexVar, if any.
   const Expr *getContainerIndexed() const { return ContainerExpr; }
 
@@ -336,6 +339,7 @@ private:
   /// A container which holds all usages of IndexVar as the index of
   /// ArraySubscriptExpressions.
   UsageResult Usages;
+  llvm::SmallSet<SourceLocation, 8> UsageLocations;
   bool OnlyUsedAsIndex;
   /// The DeclStmt for an alias to the container element.
   const DeclStmt *AliasDecl;
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
index da0a3e4..365e0f7 100644
--- a/clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
+++ b/clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp
@@ -884,3 +884,32 @@ void iterators() {
 }
 
 } // namespace Lambdas
+
+namespace InitLists {
+
+struct D { int i; };
+struct E { D d; };
+int g(int b);
+
+void f() {
+  const unsigned N = 3;
+  int Array[N];
+
+  // Subtrees of InitListExpr are visited twice. Test that we do not do repeated
+  // replacements.
+  for (unsigned i = 0; i < N; ++i) {
+    int a{ Array[i] };
+    int b{ g(Array[i]) };
+    int c{ g( { Array[i] } ) };
+    D d{ { g( { Array[i] } ) } };
+    E e{ { { g( { Array[i] } ) } } };
+  }
+  // CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: int a{ elem };
+  // CHECK-FIXES-NEXT: int b{ g(elem) };
+  // CHECK-FIXES-NEXT: int c{ g( { elem } ) };
+  // CHECK-FIXES-NEXT: D d{ { g( { elem } ) } };
+  // CHECK-FIXES-NEXT: E e{ { { g( { elem } ) } } };
+}
+
+} // namespace InitLists