Prevent loop-convert from leaving empty lines after removing an alias declaration.

[angelgarcia]

Summary

This fixes https://llvm.org/bugs/show_bug.cgi?id=17716.

[klimek]

clang-tidy/modernize/LoopConvertCheck.cpp

455–457

Replace this with something like:
We need to delete a potential newline after the deleted alias, as clang-format will leave empty lines untouched. For all other formatting we rely on clang-format to fix it.

[klimek]

apart from that LG

[angelgarcia]

Change a comment.

clang-tidy/modernize/LoopConvertCheck.h

struct RangeDescriptor {
     RangeDescriptor();
     bool ContainerNeedsDereference;
     bool DerefByConstRef;
     bool DerefByValue;
     bool IsTriviallyCopyable;
     std::string ContainerString;
   };
 
+  void getAliasRange(SourceManager &SM, SourceRange &DeclRange);
+
   void doConversion(ASTContext *Context, const VarDecl *IndexVar,
                     const VarDecl *MaybeContainer, const UsageResult &Usages,
                     const DeclStmt *AliasDecl, bool AliasUseRequired,
                     bool AliasFromForInit, const ForStmt *Loop,
                     RangeDescriptor Descriptor);
 
   StringRef getContainerString(ASTContext *Context, const ForStmt *Loop,
                                const Expr *ContainerExpr);

clang-tidy/modernize/LoopConvertCheck.cpp

void LoopConvertCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     return;
 
   Finder->addMatcher(makeArrayLoopMatcher(), this);
   Finder->addMatcher(makeIteratorLoopMatcher(), this);
   Finder->addMatcher(makePseudoArrayLoopMatcher(), this);
 }
 
+/// \brief Given the range of a single declaration, such as:
+/// \code
+///   unsigned &ThisIsADeclarationThatCanSpanSeveralLinesOfCode =
+///       InitializationValues[I];
+///   next_instruction;
+/// \endcode
+/// Finds the range that has to be erased to remove this declaration without
+/// leaving empty lines, by extending the range until the beginning of the
+/// next instruction.
+///
+/// We need to delete a potential newline after the deleted alias, as
+/// clang-format will leave empty lines untouched. For all other formatting we
+/// rely on clang-format to fix it.
+void LoopConvertCheck::getAliasRange(SourceManager &SM, SourceRange &Range) {
+  bool Invalid = false;
+  const char *TextAfter =
+      SM.getCharacterData(Range.getEnd().getLocWithOffset(1), &Invalid);
+  if (Invalid)
+    return;
+  unsigned Offset = std::strspn(TextAfter, " \t\r\n");
+  Range =
+      SourceRange(Range.getBegin(), Range.getEnd().getLocWithOffset(Offset));
+}
+
 /// \brief Computes the changes needed to convert a given for loop, and
 /// applies them.
 void LoopConvertCheck::doConversion(
     ASTContext *Context, const VarDecl *IndexVar, const VarDecl *MaybeContainer,
     const UsageResult &Usages, const DeclStmt *AliasDecl, bool AliasUseRequired,
     bool AliasFromForInit, const ForStmt *Loop, RangeDescriptor Descriptor) {
   auto Diag = diag(Loop->getForLoc(), "use range-based for loop instead");
 
   std::string VarName;
   bool VarNameFromAlias = (Usages.size() == 1) && AliasDecl;
   bool AliasVarIsRef = false;
 
   if (VarNameFromAlias) {
     const auto *AliasVar = cast<VarDecl>(AliasDecl->getSingleDecl());
     VarName = AliasVar->getName().str();
     AliasVarIsRef = AliasVar->getType()->isReferenceType();
 
     // We keep along the entire DeclStmt to keep the correct range here.
-    const SourceRange &ReplaceRange = AliasDecl->getSourceRange();
+    SourceRange ReplaceRange = AliasDecl->getSourceRange();
 
     std::string ReplacementText;
     if (AliasUseRequired) {
       ReplacementText = VarName;
     } else if (AliasFromForInit) {
       // FIXME: Clang includes the location of the ';' but only for DeclStmt's
       // in a for loop's init clause. Need to put this ';' back while removing
       // the declaration of the alias variable. This is probably a bug.
       ReplacementText = ";";
+    } else {
+      // Avoid leaving empty lines or trailing whitespaces.
+      getAliasRange(Context->getSourceManager(), ReplaceRange);
     }
 
     Diag << FixItHint::CreateReplacement(
         CharSourceRange::getTokenRange(ReplaceRange), ReplacementText);
     // No further replacements are made to the loop, since the iterator or index
     // was used exactly once - in the initialization of AliasVar.
   } else {
     VariableNamer Namer(&TUInfo->getGeneratedDecls(),

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

void aliasing() {
   for (int I = 0; I < N; ++I) {
     Val &T = Arr[I];
     {}
     int Y = T.X;
   }
   // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & T : Arr)
   // CHECK-FIXES-NOT: Val &{{[a-z_]+}} =
-  // CHECK-FIXES: {}
+  // CHECK-FIXES-NEXT: {}
   // CHECK-FIXES-NEXT: int Y = T.X;
 
   // The container was not only used to initialize a temporary loop variable for
   // the container's elements, so we do not alias the new loop variable.
   for (int I = 0; I < N; ++I) {
     Val &T = Arr[I];
     int Y = T.X;
     int Z = Arr[I].X + T.X;
void aliasing() {
   // which provide a subscript operator[].
   for (int I = 0; I < V.size(); ++I) {
     Val &T = V[I];
     {}
     int Y = T.X;
   }
   // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & T : V)
-  // CHECK-FIXES: {}
+  // CHECK-FIXES-NEXT: {}
   // CHECK-FIXES-NEXT: int Y = T.X;
 
   // The same with a call to at()
   for (int I = 0; I < Pv->size(); ++I) {
     Val &T = Pv->at(I);
     {}
     int Y = T.X;
   }
   // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & T : *Pv)
-  // CHECK-FIXES: {}
+  // CHECK-FIXES-NEXT: {}
   // CHECK-FIXES-NEXT: int Y = T.X;
 
   for (int I = 0; I < N; ++I) {
     Val &T = func(Arr[I]);
     int Y = T.X;
   }
   // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & Elem : Arr)
void aliasing() {
 
   struct IntRef { IntRef(const int& i); };
   for (int I = 0; I < N; ++I) {
     IntRef Int(IntArr[I]);
   }
   // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & Elem : IntArr)
   // CHECK-FIXES-NEXT: IntRef Int(Elem);
-}
 
+  // Ensure that removing the alias doesn't leave empty lines behind.
+  for (int I = 0; I < N; ++I) {
+    auto &X = IntArr[I];
+    X = 0;
+  }
+  // CHECK-MESSAGES: :[[@LINE-4]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto & X : IntArr) {
+  // CHECK-FIXES-NEXT: {{^    X = 0;$}}
+  // CHECK-FIXES-NEXT: {{^  }$}}
+}
 
 void refs_and_vals() {
   // The following tests check that the transform correctly preserves the
   // reference or value qualifiers of the aliased variable. That is, if the
   // variable was declared as a value, the loop variable will be declared as a
   // value and vice versa for references.
 
   S Ss;
   const S S_const = Ss;
 
   for (S::const_iterator It = S_const.begin(); It != S_const.end(); ++It) {
     MutableVal Alias = *It;
     {}
     Alias.X = 0;
   }
   // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto Alias : S_const)
   // CHECK-FIXES-NOT: MutableVal {{[a-z_]+}} =
-  // CHECK-FIXES: {}
+  // CHECK-FIXES-NEXT: {}
   // CHECK-FIXES-NEXT: Alias.X = 0;
 
   for (S::iterator It = Ss.begin(), E = Ss.end(); It != E; ++It) {
     MutableVal Alias = *It;
     {}
     Alias.X = 0;
   }
   // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto Alias : Ss)
   // CHECK-FIXES-NOT: MutableVal {{[a-z_]+}} =
-  // CHECK-FIXES: {}
+  // CHECK-FIXES-NEXT: {}
   // CHECK-FIXES-NEXT: Alias.X = 0;
 
   for (S::iterator It = Ss.begin(), E = Ss.end(); It != E; ++It) {
     MutableVal &Alias = *It;
     {}
     Alias.X = 0;
   }
   // CHECK-MESSAGES: :[[@LINE-5]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & Alias : Ss)
   // CHECK-FIXES-NOT: MutableVal &{{[a-z_]+}} =
-  // CHECK-FIXES: {}
+  // CHECK-FIXES-NEXT: {}
   // CHECK-FIXES-NEXT: Alias.X = 0;
 
   dependent<int> Dep, Other;
   for (dependent<int>::iterator It = Dep.begin(), E = Dep.end(); It != E; ++It) {
     printf("%d\n", *It);
     const int& Idx = Other[0];
     unsigned Othersize = Other.size();
   }
for (int I = 0; I < N; ++I) {
     auto G5 = [&]() {
       int R5 = Arr[I];
       int J5 = 8 + R5;
     };
   }
   // CHECK-MESSAGES: :[[@LINE-6]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto R5 : Arr)
   // CHECK-FIXES-NEXT: auto G5 = [&]()
-  // CHECK-FIXES: int J5 = 8 + R5;
+  // CHECK-FIXES-NEXT: int J5 = 8 + R5;
 
   // Alias by reference.
   for (int I = 0; I < N; ++I) {
     auto G6 = [&]() {
       int &R6 = Arr[I];
       int J6 = -1 + R6;
     };
   }
   // CHECK-MESSAGES: :[[@LINE-6]]:3: warning: use range-based for loop instead
   // CHECK-FIXES: for (auto & R6 : Arr)
   // CHECK-FIXES-NEXT: auto G6 = [&]()
-  // CHECK-FIXES: int J6 = -1 + R6;
+  // CHECK-FIXES-NEXT: int J6 = -1 + R6;
 }
 
 void iterators() {
   dependent<int> Dep;
 
   for (dependent<int>::iterator I = Dep.begin(), E = Dep.end(); I != E; ++I)
     auto H1 = [&I]() { int R = *I; };
   // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: use range-based for loop instead
void f() {
   for (unsigned I = 0; I < N; ++I) {
     int A{ Array[I] };
     int B{ g(Array[I]) };
     int C{ g( { Array[I] } ) };
     D Dd{ { g( { Array[I] } ) } };
     E Ee{ { { g( { Array[I] } ) } } };
   }
   // CHECK-MESSAGES: :[[@LINE-7]]:3: warning: use range-based for loop instead
-  // CHECK-FIXES: int A{ Elem };
+  // CHECK-FIXES: for (auto & Elem : Array)
+  // CHECK-FIXES-NEXT: int A{ Elem };
   // CHECK-FIXES-NEXT: int B{ g(Elem) };
   // CHECK-FIXES-NEXT: int C{ g( { Elem } ) };
   // CHECK-FIXES-NEXT: D Dd{ { g( { Elem } ) } };
   // CHECK-FIXES-NEXT: E Ee{ { { g( { Elem } ) } } };
 }
 
 } // namespace InitLists