Fix several corner cases for loop-convert check.

[angelgarcia]

Summary

Reduced the amount of wrong conversions of this check.

[klimek]

Nice!

clang-tidy/modernize/LoopConvertCheck.cpp

378

(not necessarily in this CL) please rename E to Expression or similar; I'm fine with one-letter variables for small scopes, but struct scopes are basically infinite.

test/clang-tidy/modernize-loop-convert-basic.cpp

437

Use LLVM coding conventions for iterators (I, E) as above.

448

This test seems to be missing the it.insert(0) case that was removed from the "unsupported" comment, if I'm not missing something.

540–541

Isn't it important that it's a pointer to an unsigned const, not that the iterator method is const?

[angelgarcia]

test/clang-tidy/modernize-loop-convert-basic.cpp

448

It was in the test 'modernize-loop-convert-negative.cpp'. I moved these ones there as well.

540–541

The important thing is that the check now adds a "const" whenever it is safe to do it. I changed the return type to a pointer just to be more consistent with what an iterator is supposed to be. It isn't important at all in this case.

[angelgarcia]

Renamed Usage::E, moved some tests and other fixes.

[klimek]

test/clang-tidy/modernize-loop-convert-basic.cpp

448

Why is that case removed from the unsupported list then?

540–542

That makes sense.

[klimek]

Learned that I need to learn to read, because the deleted comment was just duplicated.
Looks good, great first step towards making this significantly more useful :)

clang-tidy/modernize/LoopConvertCheck.cpp

 /// \brief Returns true when the given expression is a member expression
 /// whose base is `this` (implicitly or not).
 static bool isDirectMemberExpr(const Expr *E) {
   if (const auto *Member = dyn_cast<MemberExpr>(E->IgnoreParenImpCasts()))
     return isa<CXXThisExpr>(Member->getBase()->IgnoreParenImpCasts());
   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>(
                         Options.get("MinConfidence", "reasonable"))
                         .Case("safe", Confidence::CL_Safe)
                         .Case("risky", Confidence::CL_Risky)
                         .Default(Confidence::CL_Reasonable)) {}
 
if (DerefByValue) {
         AutoRefType = Context->getConstType(AutoRefType);
       AutoRefType = Context->getLValueReferenceType(AutoRefType);
     }
   }
 
   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));
 }
 
 /// \brief Determine if the change should be deferred or rejected, returning
 /// text which refers to the container iterated over if the change should
 /// proceed.
 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 "";
 
   Context->getTranslationUnitDecl();
   TUInfo->getParentFinder();
   TUInfo->getParentFinder().gatherAncestors(Context->getTranslationUnitDecl());
   // Ensure that we do not try to move an expression dependent on a local
if (FixerKind == LFK_Array) {
     // The array being indexed by IndexVar was discovered during traversal.
     ContainerExpr = Finder.getContainerIndexed()->IgnoreParenImpCasts();
     // Very few loops are over expressions that generate arrays rather than
     // array variables. Consider loops over arrays that aren't just represented
     // by a variable to be risky conversions.
     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);
 
   if (ContainerString.empty() || ConfidenceLevel.getLevel() < MinConfidence)
     return;
 
   doConversion(Context, LoopVar, getReferencedVariable(ContainerExpr),

clang-tidy/modernize/LoopConvertUtils.cpp

ForLoopIndexUseVisitor::ForLoopIndexUseVisitor(ASTContext *Context,
                                                bool ContainerNeedsDereference)
     : Context(Context), IndexVar(IndexVar), EndVar(EndVar),
       ContainerExpr(ContainerExpr), ArrayBoundExpr(ArrayBoundExpr),
       ContainerNeedsDereference(ContainerNeedsDereference),
       OnlyUsedAsIndex(true), AliasDecl(nullptr),
       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) {
   TraverseStmt(const_cast<Stmt *>(Body));
   return OnlyUsedAsIndex && ContainerExpr;
 }
 
 void ForLoopIndexUseVisitor::addComponents(const ComponentVector &Components) {

 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));
     return true;
   }
 
-  return VisitorBase::TraverseUnaryOperator(Uop);
+  return VisitorBase::TraverseUnaryDeref(Uop);
 }
 
 /// \brief If the member expression is operator-> (overloaded or not) on
 /// IndexVar, include it as a valid usage and prune the traversal.
 ///
 /// For example, given
 /// \code
 ///   struct Foo { int bar(); int x; };
if (Call->getOperator() == OO_Arrow) {
       assert(Call->getNumArgs() == 1 &&
              "Operator-> takes more than one argument");
       Obj = getDeclRef(Call->getArg(0));
       ResultExpr = Obj;
       ExprType = Call->getCallReturnType(*Context);
     }
   }
 
-  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();
 
     assert(ExprType->isPointerType() && "Operator-> returned non-pointer type");
     // FIXME: This works around not having the location of the arrow operator.
     // Consider adding OperatorLoc to MemberExpr?
     SourceLocation ArrowLoc = Lexer::getLocForEndOfToken(
         Base->getExprLoc(), 0, Context->getSourceManager(),
         Context->getLangOpts());
     // 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)));
       return true;
     }
   }
-  return TraverseStmt(Member->getBase());
+  return VisitorBase::TraverseMemberExpr(Member);
 }
 
 /// \brief If a member function call is the at() accessor on the container with
 /// IndexVar as the single argument, include it as a valid usage and prune
 /// the traversal.
 ///
 /// Member calls on other objects will not be permitted.
 /// Calls on the iterator object are not permitted, unless done through
bool ForLoopIndexUseVisitor::TraverseCXXMemberCallExpr(
 
   if (containsExpr(Context, &DependentExprs, Member->getBase()))
     ConfidenceLevel.lowerTo(Confidence::CL_Risky);
 
   return VisitorBase::TraverseCXXMemberCallExpr(MemberCall);
 }
 
 /// \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
 /// \code
 ///   struct Foo { int bar(); int x; };
 ///   vector<Foo> v;
 ///   void f(Foo);
 /// \endcode

 /// For example, these are not permitted:
 /// \code
 ///   for (int i = 0; i < N; ++i) {  printf("arr[%d] = %d", i, arr[i]); }
 ///   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)
-///     i.insert(0);
-///   for (vector<int>::iterator i = container.begin(), e = container.end();
-///        i != e; ++i)
 ///     if (i + 1 != e)
 ///       printf("%d", *i);
 /// \endcode
 ///
 /// And these will raise the risk level:
 /// \code
 ///    int arr[10][20];
 ///    int l = 5;
 ///    for (int j = 0; j < 20; ++j)
 ///      int k = arr[l][j] + l; // using l outside arr[l] is considered risky
 ///    for (int i = 0; i < obj.getVector().size(); ++i)
 ///      obj.foo(10); // using `obj` is considered risky
 /// \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);
   return true;
 }
 
 /// \brief If we find that another variable is created just to refer to the loop
 /// element, note it for reuse as the loop variable.

test/clang-tidy/modernize-loop-convert-basic.cpp

void doLoop() {
     // around it. Make sure the replacement still happens.
     for (iterator I = begin(), E = end(); I != E; ++I) {
     }
     // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: use range-based for loop instead
     // CHECK-FIXES: for (auto & elem : *this) {
   }
 };
 
+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 {
 
 const int N = 6;
 dependent<int> v;
 dependent<int> *pv;
 
void f() {
   // CHECK-FIXES-NEXT: sum += elem + 2;
 }
 
 // Check for loops that don't mention containers.
 void noContainer() {
   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 {
   unsigned size() const;
 };
 
 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
 // begin() and end() of the right const-ness.
 void NoBeginEndTest() {
   NoBeginEnd NBE;
   for (unsigned i = 0, e = NBE.size(); i < e; ++i) {
   }
 
   const NoConstBeginEnd const_NCBE;
   for (unsigned i = 0, e = const_NCBE.size(); i < e; ++i) {
   }
 
   ConstBeginEnd CBE;
   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

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

void f() {
   for (int *I = begin(C), *E = end(C); I != E; ++I) {
   }
 }
 
 } // namespace FreeBeginEnd
 
 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;
   Val Arr[N];
   for (int i = 0; i < N; ++i) {
     for (int j = 0; j < N; ++j) {
       int k = Arr[i].x + Arr[j].x;
       // The repeat is there to allow FileCheck to make sure the two variable
for (Nested<S>::const_iterator I = NestS.begin(), E = NestS.end(); I != E; ++I) {
     for (S::const_iterator SI = (*I).begin(), SE = (*I).end(); SI != SE; ++SI) {
       printf("%d", *SI);
     }
   }
   // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead
   // 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
 
 namespace SingleIterator {
 
 void complexContainer() {
   X exes[5];