From 90bf895aa3f7aaf5b39b94133d57aa618f27437b Mon Sep 17 00:00:00 2001 From: Angel Garcia Gomez Date: Thu, 1 Oct 2015 13:08:21 +0000 Subject: [PATCH] Prevent loop-convert from leaving empty lines after removing an alias declaration. Summary: This fixes https://llvm.org/bugs/show_bug.cgi?id=17716. Reviewers: klimek Subscribers: cfe-commits, alexfh Differential Revision: http://reviews.llvm.org/D13342 llvm-svn: 249006 --- .../clang-tidy/modernize/LoopConvertCheck.cpp | 29 +++++++++++++++++- .../clang-tidy/modernize/LoopConvertCheck.h | 2 ++ .../modernize-loop-convert-extra.cpp | 30 ++++++++++++------- 3 files changed, 50 insertions(+), 11 deletions(-) diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp index ebe124fc3af5..33a2d6da96a2 100644 --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp @@ -442,6 +442,30 @@ void LoopConvertCheck::registerMatchers(MatchFinder *Finder) { 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( @@ -460,7 +484,7 @@ void LoopConvertCheck::doConversion( 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) { @@ -470,6 +494,9 @@ void LoopConvertCheck::doConversion( // 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( diff --git a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h index f2bcf59691c4..0337719fac08 100644 --- a/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.h @@ -34,6 +34,8 @@ private: 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, diff --git a/clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp b/clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp index d1b50aec1250..a72e77f48b82 100644 --- a/clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp +++ b/clang-tools-extra/test/clang-tidy/modernize-loop-convert-extra.cpp @@ -53,7 +53,7 @@ void aliasing() { // 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 @@ -89,7 +89,7 @@ void aliasing() { } // 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() @@ -100,7 +100,7 @@ void aliasing() { } // 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) { @@ -166,8 +166,17 @@ void aliasing() { // 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 @@ -186,7 +195,7 @@ void refs_and_vals() { // 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) { @@ -197,7 +206,7 @@ void refs_and_vals() { // 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) { @@ -208,7 +217,7 @@ void refs_and_vals() { // 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 Dep, Other; @@ -863,7 +872,7 @@ void implicitCapture() { // 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) { @@ -875,7 +884,7 @@ void implicitCapture() { // 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() { @@ -953,7 +962,8 @@ void f() { 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 } ) } };