Remove dangling parenthesis.

[angelgarcia]

Summary

Remove parenthesis surrounding the new loop index.

[klimek]

Can you add a test where we need the parens? (where the element is of ** type or something)

[angelgarcia]

Added a test where the parenthesis must not be removed, and one where they should.

[klimek]

clang-tidy/modernize/LoopConvertCheck.cpp

482

Perhaps add a comment (or correct me if I'm wrong ;)
Usage.Expression contains the DeclRefExpr from the point of usage.
Parens around a simple DeclRefExpr can always be removed.

(can Usage.Expression be something more complex?)

[angelgarcia]

Usage.Expression can be:
-CXXMemberCallExpr: "container.at(i)"
-CXXOperatorCallExpr: "*it" or "container[i]"
-ArraySubscripExpr: "Arr[i]"
-UnaryOperator: "*ptr"
-MemberExpr: "it->member" (we only replace "it->" with "elem." in this
case).

The point is that all of them except the last one will be replaced by
"elem" (or any other identifier), so it will be a DeclRefExpr after the
change, and the parenthesis can always be removed. I can add a comment
explaining this.

[angelgarcia]

Add comment.

[klimek]

LG

clang-tidy/modernize/LoopConvertCheck.cpp

if (VarNameFromAlias) {
     VariableNamer Namer(&TUInfo->getGeneratedDecls(),
                         &TUInfo->getParentFinder().getStmtToParentStmtMap(),
                         Loop, IndexVar, MaybeContainer, Context);
     VarName = Namer.createIndexName();
     // First, replace all usages of the array subscript expression with our new
     // variable.
     for (const auto &Usage : Usages) {
       std::string ReplaceText;
+      SourceRange Range = Usage.Range;
       if (Usage.Expression) {
         // If this is an access to a member through the arrow operator, after
         // the replacement it must be accessed through the '.' operator.
         ReplaceText = Usage.Kind == Usage::UK_MemberThroughArrow ? VarName + "."
                                                                  : VarName;
+        auto Parents = Context->getParents(*Usage.Expression);
+        if (Parents.size() == 1) {
+          if (const auto *Paren = Parents[0].get<ParenExpr>())
+            Range = Paren->getSourceRange();
+        }
       } else {
         // The Usage expression is only null in case of lambda captures (which
         // are VarDecl). If the index is captured by value, add '&' to capture
         // by reference instead.
         ReplaceText =
             Usage.Kind == Usage::UK_CaptureByCopy ? "&" + VarName : VarName;
       }
       TUInfo->getReplacedVars().insert(std::make_pair(Loop, IndexVar));
       Diag << FixItHint::CreateReplacement(
-          CharSourceRange::getTokenRange(Usage.Range), ReplaceText);
+          CharSourceRange::getTokenRange(Range), ReplaceText);
     }
   }
 
   // Now, we need to construct the new range expression.
   SourceRange ParenRange(Loop->getLParenLoc(), Loop->getRParenLoc());
 
   QualType AutoType = Context->getAutoDeductType();
 

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

void f() {
   // CHECK-FIXES-NEXT: printf("I found %d\n", elem);
 
   S s;
   for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) {
     printf("s has value %d\n", (*it).x);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : s)
-  // CHECK-FIXES-NEXT: printf("s has value %d\n", (elem).x);
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", elem.x);
 
   S *ps;
   for (S::iterator it = ps->begin(), e = ps->end(); it != e; ++it) {
     printf("s has value %d\n", (*it).x);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & p : *ps)
-  // CHECK-FIXES-NEXT: printf("s has value %d\n", (p).x);
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", p.x);
 
   for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) {
     printf("s has value %d\n", it->x);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : s)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", elem.x);
 
   for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) {
     it->x = 3;
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : s)
   // CHECK-FIXES-NEXT: elem.x = 3;
 
   for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) {
     (*it).x = 3;
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : s)
-  // CHECK-FIXES-NEXT: (elem).x = 3;
+  // CHECK-FIXES-NEXT: elem.x = 3;
 
   for (S::iterator it = s.begin(), e = s.end(); it != e; ++it) {
     it->nonConstFun(4, 5);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : s)
   // CHECK-FIXES-NEXT: elem.nonConstFun(4, 5);
 
   U u;
   for (U::iterator it = u.begin(), e = u.end(); it != e; ++it) {
     printf("s has value %d\n", it->x);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : u)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", elem.x);
 
   for (U::iterator it = u.begin(), e = u.end(); it != e; ++it) {
     printf("s has value %d\n", (*it).x);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : u)
-  // CHECK-FIXES-NEXT: printf("s has value %d\n", (elem).x);
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", elem.x);
 
   U::iterator A;
   for (U::iterator i = u.begin(), e = u.end(); i != e; ++i)
     int k = A->x + i->x;
   // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : u)
   // CHECK-FIXES-NEXT: int k = A->x + elem.x;
 
void different_type() {
   // s.begin() returns a type 'iterator' which is just a non-const pointer and
   // differs from const_iterator only on the const qualification.
   S s;
   for (S::const_iterator it = s.begin(), e = s.end(); it != e; ++it) {
     printf("s has value %d\n", (*it).x);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto elem : s)
-  // CHECK-FIXES-NEXT: printf("s has value %d\n", (elem).x);
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", elem.x);
 
   S *ps;
   for (S::const_iterator it = ps->begin(), e = ps->end(); it != e; ++it) {
     printf("s has value %d\n", (*it).x);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto p : *ps)
-  // CHECK-FIXES-NEXT: printf("s has value %d\n", (p).x);
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", p.x);
 
   // v.begin() returns a user-defined type 'iterator' which, since it's
   // different from const_iterator, disqualifies these loops from
   // transformation.
   dependent<int> v;
   for (dependent<int>::const_iterator it = v.begin(), e = v.end();
        it != e; ++it) {
     printf("Fibonacci number is %d\n", *it);

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

 void macroConflict() {
   S MAXs;
   for (S::iterator it = MAXs.begin(), e = MAXs.end(); it != e; ++it) {
     printf("s has value %d\n", (*it).x);
     printf("Max of 3 and 5: %d\n", MAX(3, 5));
   }
   // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & MAXs_it : MAXs)
-  // CHECK-FIXES-NEXT: printf("s has value %d\n", (MAXs_it).x);
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", MAXs_it.x);
   // CHECK-FIXES-NEXT: printf("Max of 3 and 5: %d\n", MAX(3, 5));
 
   for (S::const_iterator it = MAXs.begin(), e = MAXs.end(); it != e; ++it) {
     printf("s has value %d\n", (*it).x);
     printf("Max of 3 and 5: %d\n", MAX(3, 5));
   }
   // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto MAXs_it : MAXs)
-  // CHECK-FIXES-NEXT: printf("s has value %d\n", (MAXs_it).x);
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", MAXs_it.x);
   // CHECK-FIXES-NEXT: printf("Max of 3 and 5: %d\n", MAX(3, 5));
 
   T DEFs;
   for (T::iterator it = DEFs.begin(), e = DEFs.end(); it != e; ++it) {
     if (*it == DEF) {
       printf("I found %d\n", *it);
     }
   }
void keywordConflict() {
 
   U __FUNCTION__s;
   for (U::iterator it = __FUNCTION__s.begin(), e = __FUNCTION__s.end();
        it != e; ++it) {
     int __FUNCTION__s_it = (*it).x + 2;
   }
   // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & __FUNCTION__s_elem : __FUNCTION__s)
-  // CHECK-FIXES-NEXT: int __FUNCTION__s_it = (__FUNCTION__s_elem).x + 2;
+  // CHECK-FIXES-NEXT: int __FUNCTION__s_it = __FUNCTION__s_elem.x + 2;
 }
 
 void typeConflict() {
   T Vals;
   // Using the name "Val", although it is the name of an existing struct, is
   // safe in this loop since it will only exist within this scope.
   for (T::iterator it = Vals.begin(), e = Vals.end(); it != e; ++it)
     (void) *it;
void f() {
   Nested<T> NestT;
   for (Nested<T>::iterator I = NestT.begin(), E = NestT.end(); I != E; ++I) {
     for (T::iterator TI = (*I).begin(), TE = (*I).end(); TI != TE; ++TI) {
       printf("%d", *TI);
     }
   }
   // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : NestT)
-  // CHECK-FIXES-NEXT: for (T::iterator TI = (elem).begin(), TE = (elem).end(); TI != TE; ++TI)
+  // CHECK-FIXES-NEXT: for (T::iterator TI = elem.begin(), TE = elem.end(); TI != TE; ++TI)
   // CHECK-FIXES-NEXT: printf("%d", *TI);
 
   // The inner loop is also convertible.
   Nested<S> NestS;
   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 (auto elem : NestS)
-  // CHECK-FIXES-NEXT: for (S::const_iterator SI = (elem).begin(), SE = (elem).end(); SI != SE; ++SI)
+  // 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);
     }
void f() {
   // CHECK-FIXES-NEXT: printf("I found %d\n", elem);
 
   S s;
   for (S::iterator it = s.begin(); it != s.end(); ++it) {
     printf("s has value %d\n", (*it).x);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : s)
-  // CHECK-FIXES-NEXT: printf("s has value %d\n", (elem).x);
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", elem.x);
 
   S *ps;
   for (S::iterator it = ps->begin(); it != ps->end(); ++it) {
     printf("s has value %d\n", (*it).x);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & p : *ps)
-  // CHECK-FIXES-NEXT: printf("s has value %d\n", (p).x);
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", p.x);
 
   for (S::iterator it = s.begin(); it != s.end(); ++it) {
     printf("s has value %d\n", it->x);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : s)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", elem.x);
 
   for (S::iterator it = s.begin(); it != s.end(); ++it) {
     it->x = 3;
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : s)
   // CHECK-FIXES-NEXT: elem.x = 3;
 
   for (S::iterator it = s.begin(); it != s.end(); ++it) {
     (*it).x = 3;
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : s)
-  // CHECK-FIXES-NEXT: (elem).x = 3;
+  // CHECK-FIXES-NEXT: elem.x = 3;
 
   for (S::iterator it = s.begin(); it != s.end(); ++it) {
     it->nonConstFun(4, 5);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : s)
   // CHECK-FIXES-NEXT: elem.nonConstFun(4, 5);
 
   U u;
   for (U::iterator it = u.begin(); it != u.end(); ++it) {
     printf("s has value %d\n", it->x);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : u)
   // CHECK-FIXES-NEXT: printf("s has value %d\n", elem.x);
 
   for (U::iterator it = u.begin(); it != u.end(); ++it) {
     printf("s has value %d\n", (*it).x);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : u)
-  // CHECK-FIXES-NEXT: printf("s has value %d\n", (elem).x);
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", elem.x);
 
   U::iterator A;
   for (U::iterator i = u.begin(); i != u.end(); ++i)
     int k = A->x + i->x;
   // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & elem : u)
   // CHECK-FIXES-NEXT: int k = A->x + elem.x;
 
void different_type() {
   // s.begin() returns a type 'iterator' which is just a non-const pointer and
   // differs from const_iterator only on the const qualification.
   S s;
   for (S::const_iterator it = s.begin(); it != s.end(); ++it) {
     printf("s has value %d\n", (*it).x);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto elem : s)
-  // CHECK-FIXES-NEXT: printf("s has value %d\n", (elem).x);
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", elem.x);
 
   S *ps;
   for (S::const_iterator it = ps->begin(); it != ps->end(); ++it) {
     printf("s has value %d\n", (*it).x);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto p : *ps)
-  // CHECK-FIXES-NEXT: printf("s has value %d\n", (p).x);
+  // CHECK-FIXES-NEXT: printf("s has value %d\n", p.x);
 
   // v.begin() returns a user-defined type 'iterator' which, since it's
   // different from const_iterator, disqualifies these loops from
   // transformation.
   dependent<int> v;
   for (dependent<int>::const_iterator it = v.begin(); it != v.end(); ++it) {
     printf("Fibonacci number is %d\n", *it);
   }