3 files changed, 48 insertions, 17 deletions

diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
index 11d5425956e..b6cb64fa952 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
@@ -364,6 +364,23 @@ static bool isDirectMemberExpr(const Expr *E) {
   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 @@ void LoopConvertCheck::doConversion(
   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 @@ void LoopConvertCheck::doConversion(
 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 @@ void LoopConvertCheck::findAndVerifyUsages(
     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 ace0aee3114..732d40bd604 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.cpp
@@ -425,12 +425,8 @@ ForLoopIndexUseVisitor::ForLoopIndexUseVisitor(ASTContext *Context,
       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 @@ bool ForLoopIndexUseVisitor::TraverseMemberExpr(MemberExpr *Member) {
     }
   }
 
-  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 @@ bool ForLoopIndexUseVisitor::TraverseMemberExpr(MemberExpr *Member) {
       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 @@ bool ForLoopIndexUseVisitor::TraverseCXXMemberCallExpr(
 }
 
 /// \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 @@ bool ForLoopIndexUseVisitor::TraverseArraySubscriptExpr(ArraySubscriptExpr *E) {
 ///     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 @@ bool ForLoopIndexUseVisitor::TraverseArraySubscriptExpr(ArraySubscriptExpr *E) {
 /// \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 47da6656928..ec29b67db46 100644
--- a/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
+++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertUtils.h
@@ -197,14 +197,14 @@ private:
 /// \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.