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.

519–521

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.

519–521

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?

519–521

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.Expression->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.h

private:
   bool VisitNamedDecl(clang::NamedDecl *D);
   bool VisitDeclRefExpr(clang::DeclRefExpr *D);
   bool VisitTypeLoc(clang::TypeLoc TL);
 };
 
 /// \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.
 class Confidence {
 public:
   enum Level {
     // Transformations that are likely to change semantics.
     CL_Risky,

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) {
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

 
 } // namespace Iterator
 
 namespace PseudoArray {
 
 const int N = 6;
 dependent<int> v;
 dependent<int> *pv;
 
 transparent<dependent<int>> cv;
 
 void f() {
   int sum = 0;
   for (int i = 0, e = v.size(); i < e; ++i) {
     printf("Fibonacci number is %d\n", v[i]);
     sum += v[i] + 2;
   }
   // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : v)
   // CHECK-FIXES-NEXT: printf("Fibonacci number is %d\n", elem);
   // CHECK-FIXES-NEXT: sum += elem + 2;
 
   for (int i = 0, e = v.size(); i < e; ++i) {
     printf("Fibonacci number is %d\n", v.at(i));
     sum += v.at(i) + 2;
   }
   // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : v)
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];

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

 // RUN: %python %S/check_clang_tidy.py %s modernize-loop-convert %t -- -std=c++11 -I %S/Inputs/modernize-loop-convert
 
 #include "structures.h"
 
 // CHECK-FIXES-NOT: for ({{.*[^:]:[^:].*}})
+// CHECK-MESSAGES-NOT: modernize-loop-convert
 
 namespace Negative {
 
 const int N = 6;
 int arr[N] = {1, 2, 3, 4, 5, 6};
 int (*pArr)[N] = &arr;
-int sum = 0;
+int Sum = 0;
 
-// Checks for the index start and end:
-void indexStartAndEnd() {
-  for (int i = 0; i < N + 1; ++i)
-    sum += arr[i];
+// Checks for the Index start and end:
+void IndexStartAndEnd() {
+  for (int I = 0; I < N + 1; ++I)
+    Sum += arr[I];
 
-  for (int i = 0; i < N - 1; ++i)
-    sum += arr[i];
+  for (int I = 0; I < N - 1; ++I)
+    Sum += arr[I];
 
-  for (int i = 1; i < N; ++i)
-    sum += arr[i];
+  for (int I = 1; I < N; ++I)
+    Sum += arr[I];
 
-  for (int i = 1; i < N; ++i)
-    sum += arr[i];
+  for (int I = 1; I < N; ++I)
+    Sum += arr[I];
 
-  for (int i = 0;; ++i)
-    sum += (*pArr)[i];
+  for (int I = 0;; ++I)
+    Sum += (*pArr)[I];
 }
 
 // Checks for invalid increment steps:
 void increment() {
-  for (int i = 0; i < N; --i)
-    sum += arr[i];
+  for (int I = 0; I < N; --I)
+    Sum += arr[I];
 
-  for (int i = 0; i < N; i)
-    sum += arr[i];
+  for (int I = 0; I < N; I)
+    Sum += arr[I];
 
-  for (int i = 0; i < N;)
-    sum += arr[i];
+  for (int I = 0; I < N;)
+    Sum += arr[I];
 
-  for (int i = 0; i < N; i += 2)
-    sum++;
+  for (int I = 0; I < N; I += 2)
+    Sum++;
 }
 
-// Checks to make sure that the index isn't used outside of the array:
-void indexUse() {
-  for (int i = 0; i < N; ++i)
-    arr[i] += 1 + i;
+// Checks to make sure that the Index isn't used outside of the array:
+void IndexUse() {
+  for (int I = 0; I < N; ++I)
+    arr[I] += 1 + I;
 }
 
 // Check for loops that don't mention arrays
 void noArray() {
-  for (int i = 0; i < N; ++i)
-    sum += i;
+  for (int I = 0; I < N; ++I)
+    Sum += I;
 
-  for (int i = 0; i < N; ++i) {
+  for (int I = 0; I < N; ++I) {
   }
 
-  for (int i = 0; i < N; ++i)
+  for (int I = 0; I < N; ++I)
     ;
 }
 
 // Checks for incorrect loop variables.
 void mixedVariables() {
   int badIndex;
-  for (int i = 0; badIndex < N; ++i)
-    sum += arr[i];
+  for (int I = 0; badIndex < N; ++I)
+    Sum += arr[I];
 
-  for (int i = 0; i < N; ++badIndex)
-    sum += arr[i];
+  for (int I = 0; I < N; ++badIndex)
+    Sum += arr[I];
 
-  for (int i = 0; badIndex < N; ++badIndex)
-    sum += arr[i];
+  for (int I = 0; badIndex < N; ++badIndex)
+    Sum += arr[I];
 
-  for (int i = 0; badIndex < N; ++badIndex)
-    sum += arr[badIndex];
+  for (int I = 0; badIndex < N; ++badIndex)
+    Sum += arr[badIndex];
 }
 
-// Checks for multiple arrays indexed.
+// Checks for multiple arrays Indexed.
 void multipleArrays() {
   int badArr[N];
 
-  for (int i = 0; i < N; ++i)
-    sum += arr[i] + badArr[i];
+  for (int I = 0; I < N; ++I)
+    Sum += arr[I] + badArr[I];
 
-  for (int i = 0; i < N; ++i) {
-    int k = badArr[i];
-    sum += arr[i] + k;
+  for (int I = 0; I < N; ++I) {
+    int K = badArr[I];
+    Sum += arr[I] + K;
   }
 }
 
 struct HasArr {
   int Arr[N];
   Val ValArr[N];
 };
 
 struct HasIndirectArr {
   HasArr HA;
   void implicitThis() {
-    for (int i = 0; i < N; ++i) {
-      printf("%d", HA.Arr[i]);
+    for (int I = 0; I < N; ++I) {
+      printf("%d", HA.Arr[I]);
     }
 
-    for (int i = 0; i < N; ++i) {
-      printf("%d", HA.ValArr[i].x);
+    for (int I = 0; I < N; ++I) {
+      printf("%d", HA.ValArr[I].x);
     }
   }
 
   void explicitThis() {
-    for (int i = 0; i < N; ++i) {
-      printf("%d", this->HA.Arr[i]);
+    for (int I = 0; I < N; ++I) {
+      printf("%d", this->HA.Arr[I]);
     }
 
-    for (int i = 0; i < N; ++i) {
-      printf("%d", this->HA.ValArr[i].x);
+    for (int I = 0; I < N; ++I) {
+      printf("%d", this->HA.ValArr[I].x);
     }
   }
 };
 }
 
 namespace NegativeIterator {
 
 S s;
 T t;
 U u;
 
 struct BadBeginEnd : T {
   iterator notBegin();
   iterator notEnd();
 };
 
 void notBeginOrEnd() {
   BadBeginEnd Bad;
-  for (T::iterator i = Bad.notBegin(), e = Bad.end(); i != e; ++i)
-    int k = *i;
+  for (T::iterator I = Bad.notBegin(), E = Bad.end();  I != E; ++I)
+    int K = *I;
 
-  for (T::iterator i = Bad.begin(), e = Bad.notEnd(); i != e; ++i)
-    int k = *i;
+  for (T::iterator I = Bad.begin(), E = Bad.notEnd();  I != E; ++I)
+    int K = *I;
 }
 
 void badLoopShapes() {
-  for (T::iterator i = t.begin(), e = t.end(), f = e; i != e; ++i)
-    int k = *i;
+  for (T::iterator I = t.begin(), E = t.end(), F = E;  I != E; ++I)
+    int K = *I;
 
-  for (T::iterator i = t.begin(), e = t.end(); i != e;)
-    int k = *i;
+  for (T::iterator I = t.begin(), E = t.end();  I != E;)
+    int K = *I;
 
-  for (T::iterator i = t.begin(), e = t.end();; ++i)
-    int k = *i;
+  for (T::iterator I = t.begin(), E = t.end();; ++I)
+    int K = *I;
 
   T::iterator outsideI;
   T::iterator outsideE;
 
   for (; outsideI != outsideE; ++outsideI)
-    int k = *outsideI;
+    int K = *outsideI;
 }
 
 void iteratorArrayMix() {
   int lower;
   const int N = 6;
-  for (T::iterator i = t.begin(), e = t.end(); lower < N; ++i)
-    int k = *i;
+  for (T::iterator I = t.begin(), E = t.end(); lower < N; ++I)
+    int K = *I;
 
-  for (T::iterator i = t.begin(), e = t.end(); lower < N; ++lower)
-    int k = *i;
+  for (T::iterator I = t.begin(), E = t.end(); lower < N; ++lower)
+    int K = *I;
 }
 
 struct ExtraConstructor : T::iterator {
   ExtraConstructor(T::iterator, int);
   explicit ExtraConstructor(T::iterator);
 };
 
 void badConstructor() {
-  for (T::iterator i = ExtraConstructor(t.begin(), 0), e = t.end();
-       i != e; ++i)
-    int k = *i;
-  for (T::iterator i = ExtraConstructor(t.begin()), e = t.end(); i != e; ++i)
-    int k = *i;
+  for (T::iterator I = ExtraConstructor(t.begin(), 0), E = t.end();
+        I != E; ++I)
+    int K = *I;
+  for (T::iterator I = ExtraConstructor(t.begin()), E = t.end();  I != E; ++I)
+    int K = *I;
+}
+
+void foo(S::iterator It) {}
+class Foo {public: void bar(S::iterator It); };
+Foo fo;
+
+void iteratorUsed() {
+  for (S::iterator I = s.begin(), E = s.end();  I != E; ++I)
+    foo(I);
+
+  for (S::iterator I = s.begin(), E = s.end();  I != E; ++I)
+    fo.bar(I);
+
+  S::iterator Ret;
+  for (S::iterator I = s.begin(), E = s.end();  I != E; ++I)
+    Ret = I;
 }
 
 void iteratorMemberUsed() {
-  for (T::iterator i = t.begin(), e = t.end(); i != e; ++i)
-    i.x = *i;
+  for (T::iterator I = t.begin(), E = t.end();  I != E; ++I)
+    I.x = *I;
 
-  for (T::iterator i = t.begin(), e = t.end(); i != e; ++i)
-    int k = i.x + *i;
+  for (T::iterator I = t.begin(), E = t.end();  I != E; ++I)
+    int K = I.x + *I;
 
-  for (T::iterator i = t.begin(), e = t.end(); i != e; ++i)
-    int k = e.x + *i;
+  for (T::iterator I = t.begin(), E = t.end();  I != E; ++I)
+    int K = E.x + *I;
 }
 
 void iteratorMethodCalled() {
-  for (T::iterator i = t.begin(), e = t.end(); i != e; ++i)
-    i.insert(3);
+  for (T::iterator I = t.begin(), E = t.end();  I != E; ++I)
+    I.insert(3);
 
-  for (T::iterator i = t.begin(), e = t.end(); i != e; ++i)
-    if (i != i)
-      int k = 3;
+  for (T::iterator I = t.begin(), E = t.end();  I != E; ++I)
+    if (I != I)
+      int K = 3;
 }
 
 void iteratorOperatorCalled() {
-  for (T::iterator i = t.begin(), e = t.end(); i != e; ++i)
-    int k = *(++i);
+  for (T::iterator I = t.begin(), E = t.end();  I != E; ++I)
+    int K = *(++I);
 
-  for (S::iterator i = s.begin(), e = s.end(); i != e; ++i)
-    MutableVal k = *(++i);
+  for (S::iterator I = s.begin(), E = s.end();  I != E; ++I)
+    MutableVal K = *(++I);
 }
 
 void differentContainers() {
   T other;
-  for (T::iterator i = t.begin(), e = other.end(); i != e; ++i)
-    int k = *i;
+  for (T::iterator I = t.begin(), E = other.end();  I != E; ++I)
+    int K = *I;
 
-  for (T::iterator i = other.begin(), e = t.end(); i != e; ++i)
-    int k = *i;
+  for (T::iterator I = other.begin(), E = t.end();  I != E; ++I)
+    int K = *I;
 
   S otherS;
-  for (S::iterator i = s.begin(), e = otherS.end(); i != e; ++i)
-    MutableVal k = *i;
+  for (S::iterator I = s.begin(), E = otherS.end();  I != E; ++I)
+    MutableVal K = *I;
 
-  for (S::iterator i = otherS.begin(), e = s.end(); i != e; ++i)
-    MutableVal k = *i;
+  for (S::iterator I = otherS.begin(), E = s.end();  I != E; ++I)
+    MutableVal K = *I;
 }
 
 void wrongIterators() {
   T::iterator other;
-  for (T::iterator i = t.begin(), e = t.end(); i != other; ++i)
-    int k = *i;
+  for (T::iterator I = t.begin(), E = t.end(); I != other; ++I)
+    int K = *I;
 }
 
 struct EvilArrow : U {
   // Please, no one ever write code like this.
   U *operator->();
 };
 
 void differentMemberAccessTypes() {
   EvilArrow A;
-  for (EvilArrow::iterator i = A.begin(), e = A->end(); i != e; ++i)
-    Val k = *i;
-  for (EvilArrow::iterator i = A->begin(), e = A.end(); i != e; ++i)
-    Val k = *i;
+  for (EvilArrow::iterator I = A.begin(), E = A->end();  I != E; ++I)
+    Val K = *I;
+  for (EvilArrow::iterator I = A->begin(), E = A.end();  I != E; ++I)
+    Val K = *I;
 }
 
 void f(const T::iterator &it, int);
 void f(const T &it, int);
 void g(T &it, int);
 
 void iteratorPassedToFunction() {
-  for (T::iterator i = t.begin(), e = t.end(); i != e; ++i)
-    f(i, *i);
+  for (T::iterator I = t.begin(), E = t.end();  I != E; ++I)
+    f(I, *I);
 }
 
 // FIXME: These tests can be removed if this tool ever does enough analysis to
 // decide that this is a safe transformation. Until then, we don't want it
 // applied.
 void iteratorDefinedOutside() {
-  T::iterator theEnd = t.end();
-  for (T::iterator i = t.begin(); i != theEnd; ++i)
-    int k = *i;
-
-  T::iterator theBegin = t.begin();
-  for (T::iterator e = t.end(); theBegin != e; ++theBegin)
-    int k = *theBegin;
+  T::iterator TheEnd = t.end();
+  for (T::iterator I = t.begin(); I != TheEnd; ++I)
+    int K = *I;
+
+  T::iterator TheBegin = t.begin();
+  for (T::iterator E = t.end(); TheBegin != E; ++TheBegin)
+    int K = *TheBegin;
 }
 
 } // namespace NegativeIterator
 
 namespace NegativePseudoArray {
 
 const int N = 6;
 dependent<int> v;
 dependent<int> *pv;
 
 transparent<dependent<int>> cv;
-int sum = 0;
+int Sum = 0;
 
-// Checks for the index start and end:
-void indexStartAndEnd() {
-  for (int i = 0; i < v.size() + 1; ++i)
-    sum += v[i];
+// Checks for the Index start and end:
+void IndexStartAndEnd() {
+  for (int I = 0; I < v.size() + 1; ++I)
+    Sum += v[I];
 
-  for (int i = 0; i < v.size() - 1; ++i)
-    sum += v[i];
+  for (int I = 0; I < v.size() - 1; ++I)
+    Sum += v[I];
 
-  for (int i = 1; i < v.size(); ++i)
-    sum += v[i];
+  for (int I = 1; I < v.size(); ++I)
+    Sum += v[I];
 
-  for (int i = 1; i < v.size(); ++i)
-    sum += v[i];
+  for (int I = 1; I < v.size(); ++I)
+    Sum += v[I];
 
-  for (int i = 0;; ++i)
-    sum += (*pv)[i];
+  for (int I = 0;; ++I)
+    Sum += (*pv)[I];
 }
 
 // Checks for invalid increment steps:
 void increment() {
-  for (int i = 0; i < v.size(); --i)
-    sum += v[i];
+  for (int I = 0; I < v.size(); --I)
+    Sum += v[I];
 
-  for (int i = 0; i < v.size(); i)
-    sum += v[i];
+  for (int I = 0; I < v.size(); I)
+    Sum += v[I];
 
-  for (int i = 0; i < v.size();)
-    sum += v[i];
+  for (int I = 0; I < v.size();)
+    Sum += v[I];
 
-  for (int i = 0; i < v.size(); i += 2)
-    sum++;
+  for (int I = 0; I < v.size(); I += 2)
+    Sum++;
 }
 
-// Checks to make sure that the index isn't used outside of the container:
-void indexUse() {
-  for (int i = 0; i < v.size(); ++i)
-    v[i] += 1 + i;
+// Checks to make sure that the Index isn't used outside of the container:
+void IndexUse() {
+  for (int I = 0; I < v.size(); ++I)
+    v[I] += 1 + I;
 }
 
 // Checks for incorrect loop variables.
 void mixedVariables() {
   int badIndex;
-  for (int i = 0; badIndex < v.size(); ++i)
-    sum += v[i];
+  for (int I = 0; badIndex < v.size(); ++I)
+    Sum += v[I];
 
-  for (int i = 0; i < v.size(); ++badIndex)
-    sum += v[i];
+  for (int I = 0; I < v.size(); ++badIndex)
+    Sum += v[I];
 
-  for (int i = 0; badIndex < v.size(); ++badIndex)
-    sum += v[i];
+  for (int I = 0; badIndex < v.size(); ++badIndex)
+    Sum += v[I];
 
-  for (int i = 0; badIndex < v.size(); ++badIndex)
-    sum += v[badIndex];
+  for (int I = 0; badIndex < v.size(); ++badIndex)
+    Sum += v[badIndex];
 }
 
-// Checks for an array indexed in addition to the container.
+// Checks for an array Indexed in addition to the container.
 void multipleArrays() {
   int badArr[N];
 
-  for (int i = 0; i < v.size(); ++i)
-    sum += v[i] + badArr[i];
+  for (int I = 0; I < v.size(); ++I)
+    Sum += v[I] + badArr[I];
 
-  for (int i = 0; i < v.size(); ++i)
-    sum += badArr[i];
+  for (int I = 0; I < v.size(); ++I)
+    Sum += badArr[I];
 
-  for (int i = 0; i < v.size(); ++i) {
-    int k = badArr[i];
-    sum += k + 2;
+  for (int I = 0; I < v.size(); ++I) {
+    int K = badArr[I];
+    Sum += K + 2;
   }
 
-  for (int i = 0; i < v.size(); ++i) {
-    int k = badArr[i];
-    sum += v[i] + k;
+  for (int I = 0; I < v.size(); ++I) {
+    int K = badArr[I];
+    Sum += v[I] + K;
   }
 }
 
-// Checks for multiple containers being indexed container.
+// Checks for multiple containers being Indexed container.
 void multipleContainers() {
   dependent<int> badArr;
 
-  for (int i = 0; i < v.size(); ++i)
-    sum += v[i] + badArr[i];
+  for (int I = 0; I < v.size(); ++I)
+    Sum += v[I] + badArr[I];
 
-  for (int i = 0; i < v.size(); ++i)
-    sum += badArr[i];
+  for (int I = 0; I < v.size(); ++I)
+    Sum += badArr[I];
 
-  for (int i = 0; i < v.size(); ++i) {
-    int k = badArr[i];
-    sum += k + 2;
+  for (int I = 0; I < v.size(); ++I) {
+    int K = badArr[I];
+    Sum += K + 2;
   }
 
-  for (int i = 0; i < v.size(); ++i) {
-    int k = badArr[i];
-    sum += v[i] + k;
+  for (int I = 0; I < v.size(); ++I) {
+    int K = badArr[I];
+    Sum += v[I] + K;
   }
 }
 
 // Check to make sure that dereferenced pointers-to-containers behave nicely.
 void derefContainer() {
   // Note the dependent<T>::operator*() returns another dependent<T>.
   // This test makes sure that we don't allow an arbitrary number of *'s.
-  for (int i = 0; i < pv->size(); ++i)
-    sum += (**pv).at(i);
+  for (int I = 0; I < pv->size(); ++I)
+    Sum += (**pv).at(I);
 
-  for (int i = 0; i < pv->size(); ++i)
-    sum += (**pv)[i];
+  for (int I = 0; I < pv->size(); ++I)
+    Sum += (**pv)[I];
 }
 
 void wrongEnd() {
-  int bad;
-  for (int i = 0, e = v.size(); i < bad; ++i)
-    sum += v[i];
+  int Bad;
+  for (int I = 0, E = v.size(); I < Bad; ++I)
+    Sum += v[I];
 }
 
 // Checks to see that non-const member functions are not called on the container
 // object.
 // These could be conceivably allowed with a lower required confidence level.
 void memberFunctionCalled() {
-  for (int i = 0; i < v.size(); ++i) {
-    sum += v[i];
+  for (int I = 0; I < v.size(); ++I) {
+    Sum += v[I];
     v.foo();
   }
 
-  for (int i = 0; i < v.size(); ++i) {
-    sum += v[i];
-    dependent<int>::iterator it = v.begin();
+  for (int I = 0; I < v.size(); ++I) {
+    Sum += v[I];
+    dependent<int>::iterator It = v.begin();
   }
 }
 
 } // namespace NegativePseudoArray
 
 namespace NegativeMultiEndCall {
 
 S s;
 T t;
 U u;
 
 void f(X);
 void f(S);
 void f(T);
 
 void complexContainer() {
   X x;
-  for (S::iterator i = x.s.begin(), e = x.s.end(); i != e; ++i) {
+  for (S::iterator I = x.s.begin(), E = x.s.end();  I != E; ++I) {
     f(x);
-    MutableVal k = *i;
+    MutableVal K = *I;
   }
 
-  for (T::iterator i = x.t.begin(), e = x.t.end(); i != e; ++i) {
+  for (T::iterator I = x.t.begin(), E = x.t.end();  I != E; ++I) {
     f(x);
-    int k = *i;
+    int K = *I;
   }
 
-  for (S::iterator i = x.s.begin(), e = x.s.end(); i != e; ++i) {
+  for (S::iterator I = x.s.begin(), E = x.s.end();  I != E; ++I) {
     f(x.s);
-    MutableVal k = *i;
+    MutableVal K = *I;
   }
 
-  for (T::iterator i = x.t.begin(), e = x.t.end(); i != e; ++i) {
+  for (T::iterator I = x.t.begin(), E = x.t.end();  I != E; ++I) {
     f(x.t);
-    int k = *i;
+    int K = *I;
   }
 
-  for (S::iterator i = x.getS().begin(), e = x.getS().end(); i != e; ++i) {
+  for (S::iterator I = x.getS().begin(), E = x.getS().end();  I != E; ++I) {
     f(x.getS());
-    MutableVal k = *i;
+    MutableVal K = *I;
   }
 
-  X exes[5];
-  int index = 0;
+  X Exes[5];
+  int Index = 0;
 
-  for (S::iterator i = exes[index].getS().begin(),
-                   e = exes[index].getS().end();
-       i != e; ++i) {
-    index++;
-    MutableVal k = *i;
+  for (S::iterator I = Exes[Index].getS().begin(),
+                   E = Exes[Index].getS().end();
+        I != E; ++I) {
+    Index++;
+    MutableVal K = *I;
   }
 }
 
 } // namespace NegativeMultiEndCall