Index: clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp =================================================================== --- clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp +++ clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp @@ -81,13 +81,13 @@ if (Previous != Block->body_rend()) Start = Lexer::findLocationAfterToken( dyn_cast(*Previous)->getEndLoc(), tok::semi, SM, getLangOpts(), - /*SkipTrailingWhitespaceAndNewLine=*/true); + /*SkipTrailingWhitespaceAndNewLine=*/false); if (!Start.isValid()) Start = StmtRange.getBegin(); auto RemovedRange = CharSourceRange::getCharRange( Start, Lexer::findLocationAfterToken( StmtRange.getEnd(), tok::semi, SM, getLangOpts(), - /*SkipTrailingWhitespaceAndNewLine=*/true)); + /*SkipTrailingWhitespaceAndNewLine=*/false)); diag(StmtRange.getBegin(), Diag) << FixItHint::CreateRemoval(RemovedRange); } Index: clang-tools-extra/test/clang-tidy/checkers/readability-redundant-control-flow.cpp =================================================================== --- clang-tools-extra/test/clang-tidy/checkers/readability-redundant-control-flow.cpp +++ clang-tools-extra/test/clang-tidy/checkers/readability-redundant-control-flow.cpp @@ -10,6 +10,15 @@ // CHECK-FIXES: {{^}}void f() {{{$}} // CHECK-FIXES-NEXT: {{^ *}$}} +// Don't pull function closing brace up with comment as line is not blank. +void f_with_note() { + /* NOTE */ return; +} +// CHECK-MESSAGES: :[[@LINE-2]]:14: warning: redundant return statement at the end of a function with a void return type [readability-redundant-control-flow] +// CHECK-FIXES: {{^}}void f_with_note() {{{$}} +// CHECK-FIXES-NEXT: {{^}} /* NOTE */ {{$}} +// CHECK-FIXES-NEXT: {{^}}}{{$}} + void g() { f(); return; @@ -214,6 +223,18 @@ // CHECK-FIXES: {{^}} for (int i = 0; i < end; ++i) {{{$}} // CHECK-FIXES-NEXT: {{^ *}$}} +// Don't pull loop closing brace up with comment as line is not blank. +template +void template_loop_with_note(T end) { + for (T i = 0; i < end; ++i) { + /* NOTE */ continue; + } +} +// CHECK-MESSAGES: :[[@LINE-3]]:16: warning: redundant continue statement +// CHECK-FIXES: {{^}} for (T i = 0; i < end; ++i) {{{$}} +// CHECK-FIXES-NEXT: {{^}} /* NOTE */ {{$}} +// CHECK-FIXES-NEXT: {{^}} }{{$}} + void call_templates() { template_return(10); template_return(10.0f); @@ -221,4 +242,5 @@ template_loop(10); template_loop(10L); template_loop(10U); + template_loop_with_note(10U); } Index: clang-tools-extra/test/clang-tidy/checkers/readability-redundant-declaration.cpp =================================================================== --- clang-tools-extra/test/clang-tidy/checkers/readability-redundant-declaration.cpp +++ clang-tools-extra/test/clang-tidy/checkers/readability-redundant-declaration.cpp @@ -94,3 +94,10 @@ // CHECK-MESSAGES-NOMSCOMPAT: :[[@LINE-1]]:20: warning: redundant 'g' declaration // CHECK-FIXES-NOMSCOMPAT: {{^}}// extern g{{$}} #endif + +// Test that entire next line is removed. +inline void g(); +// Test that entire previous line is removed. +// CHECK-MESSAGES: :[[@LINE-2]]:13: warning: redundant 'g' declaration +// CHECK-FIXES: {{^}}// Test that entire next line is removed.{{$}} +// CHECK-FIXES-NEXT: {{^}}// Test that entire previous line is removed.{{$}} Index: clang-tools-extra/test/clang-tidy/checkers/readability-redundant-member-init.cpp =================================================================== --- clang-tools-extra/test/clang-tidy/checkers/readability-redundant-member-init.cpp +++ clang-tools-extra/test/clang-tidy/checkers/readability-redundant-member-init.cpp @@ -116,6 +116,34 @@ }; }; +// Multiple members, removing them does not leave blank lines +struct F10 { + F10() : + f(), + g(), + h() + {} + // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: initializer for member 'f' is redundant + // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: initializer for member 'g' is redundant + // CHECK-MESSAGES: :[[@LINE-4]]:5: warning: initializer for member 'h' is redundant + // CHECK-FIXES: F10() + // CHECK-FIXES-NEXT: {{^}} {}{{$}} + + S f, g, h; +}; + +// Constructor outside of class, remove redundant initializer leaving no blank line +struct F11 { + F11(); + S a; +}; +F11::F11() +: a() +{} +// CHECK-MESSAGES: :[[@LINE-2]]:3: warning: initializer for member 'a' is redundant +// CHECK-FIXES: {{^}}F11::F11(){{$}} +// CHECK-FIXES-NEXT: {{^}}{}{{$}} + // Initializer not written struct NF1 { NF1() {} Index: clang/include/clang/AST/CommentLexer.h =================================================================== --- clang/include/clang/AST/CommentLexer.h +++ clang/include/clang/AST/CommentLexer.h @@ -21,6 +21,13 @@ #include "llvm/Support/raw_ostream.h" namespace clang { + +/// Requires that BufferPtr point to a newline character (/n or /r). +/// Returns a pointer past the end of any platform newline, i.e. past +/// "/n", "/r", or "/r/n", up to BufferEnd. +/// Consider moving this useful function to a more general utility location. +const char *skipNewline(const char *BufferPtr, const char *BufferEnd); + namespace comments { class Lexer; Index: clang/include/clang/AST/CommentParser.h =================================================================== --- clang/include/clang/AST/CommentParser.h +++ clang/include/clang/AST/CommentParser.h @@ -22,6 +22,10 @@ namespace clang { class SourceManager; +/// Returns true if and only if S consists entirely of whitespace. +/// Consider moving this useful function to a more general utility location. +bool isWhitespace(llvm::StringRef S); + namespace comments { class CommandTraits; Index: clang/include/clang/Format/Format.h =================================================================== --- clang/include/clang/Format/Format.h +++ clang/include/clang/Format/Format.h @@ -2251,6 +2251,13 @@ formatReplacements(StringRef Code, const tooling::Replacements &Replaces, const FormatStyle &Style); +/// Examines Replaces for consecutive sequences of one or more deletions on +/// the same line that would leave a previously non-blank line blank. Returns +/// extended Replacements that fully remove each such newly blank line, +/// including trailing newline character(s), to avoid introducing blank lines. +llvm::Expected +removeNewlyBlankLines(StringRef Code, const tooling::Replacements &Replaces); + /// Returns the replacements corresponding to applying \p Replaces and /// cleaning up the code after that on success; otherwise, return an llvm::Error /// carrying llvm::StringError. Index: clang/lib/AST/CommentLexer.cpp =================================================================== --- clang/lib/AST/CommentLexer.cpp +++ clang/lib/AST/CommentLexer.cpp @@ -16,6 +16,23 @@ #include "llvm/Support/ErrorHandling.h" namespace clang { + +// Consider moving this useful function to a more general utility location. +const char *skipNewline(const char *BufferPtr, const char *BufferEnd) { + if (BufferPtr == BufferEnd) + return BufferPtr; + + if (*BufferPtr == '\n') + BufferPtr++; + else { + assert(*BufferPtr == '\r'); + BufferPtr++; + if (BufferPtr != BufferEnd && *BufferPtr == '\n') + BufferPtr++; + } + return BufferPtr; +} + namespace comments { void Token::dump(const Lexer &L, const SourceManager &SM) const { @@ -131,21 +148,6 @@ return BufferEnd; } -const char *skipNewline(const char *BufferPtr, const char *BufferEnd) { - if (BufferPtr == BufferEnd) - return BufferPtr; - - if (*BufferPtr == '\n') - BufferPtr++; - else { - assert(*BufferPtr == '\r'); - BufferPtr++; - if (BufferPtr != BufferEnd && *BufferPtr == '\n') - BufferPtr++; - } - return BufferPtr; -} - const char *skipNamedCharacterReference(const char *BufferPtr, const char *BufferEnd) { for ( ; BufferPtr != BufferEnd; ++BufferPtr) { Index: clang/lib/AST/CommentParser.cpp =================================================================== --- clang/lib/AST/CommentParser.cpp +++ clang/lib/AST/CommentParser.cpp @@ -16,7 +16,8 @@ namespace clang { -static inline bool isWhitespace(llvm::StringRef S) { +// Consider moving this useful function to a more general utility location. +bool isWhitespace(llvm::StringRef S) { for (StringRef::const_iterator I = S.begin(), E = S.end(); I != E; ++I) { if (!isWhitespace(*I)) return false; Index: clang/lib/Format/Format.cpp =================================================================== --- clang/lib/Format/Format.cpp +++ clang/lib/Format/Format.cpp @@ -25,6 +25,9 @@ #include "UnwrappedLineParser.h" #include "UsingDeclarationsSorter.h" #include "WhitespaceManager.h" +#include "clang/AST/CommentLexer.h" +#include "clang/AST/CommentParser.h" +#include "clang/Basic/CharInfo.h" #include "clang/Basic/Diagnostic.h" #include "clang/Basic/DiagnosticOptions.h" #include "clang/Basic/SourceManager.h" @@ -2356,6 +2359,85 @@ } // anonymous namespace +llvm::Expected +removeNewlyBlankLines(StringRef Code, const tooling::Replacements &Replaces) { + tooling::Replacements NewReplaces(Replaces); + // pair< LineStartPos, CheckedThroughPos > of lines that have been checked + // and confirmed that the replacement result so far will be entirely blank. + std::list> PotentialWholeLineCuts; + int LineStartPos = -1; + int LineCheckedThroughPos = -1; + bool LineBlankSoFar = true; + const char *FileText = Code.data(); + StringRef FilePath; // Must be the same for every Replacement + for (const auto &R : Replaces) { + assert(FilePath.empty() || FilePath == R.getFilePath()); + FilePath = R.getFilePath(); + const int RStartPos = R.getOffset(); + + int CurrentRLineStartPos = RStartPos; + while (CurrentRLineStartPos > 0 && + !isVerticalWhitespace(FileText[CurrentRLineStartPos - 1])) { + --CurrentRLineStartPos; + } + + assert(CurrentRLineStartPos >= LineStartPos); + if (CurrentRLineStartPos != LineStartPos) { + // We've moved on to a new line. Wrap up the old one before moving on. + if (LineBlankSoFar) { + PotentialWholeLineCuts.push_back( + std::make_pair(LineStartPos, LineCheckedThroughPos)); + } + LineCheckedThroughPos = CurrentRLineStartPos; + LineStartPos = CurrentRLineStartPos; + LineBlankSoFar = true; + } + + // Check to see if line from LineCheckedThroughPos to here is blank. + assert(RStartPos >= LineCheckedThroughPos); + StringRef PriorTextToCheck(FileText + LineCheckedThroughPos, + RStartPos - LineCheckedThroughPos); + StringRef ReplacementText = R.getReplacementText(); + LineBlankSoFar = LineBlankSoFar && isWhitespace(PriorTextToCheck) && + ReplacementText.empty(); + LineCheckedThroughPos = R.getOffset() + R.getLength(); + } + + if (LineBlankSoFar) { + PotentialWholeLineCuts.push_back( + std::make_pair(LineStartPos, LineCheckedThroughPos)); + } + + // Now remove whole line if and only if (a) rest of line is blank, and + // (b) the original line was *not* blank. + for (const auto &LineCheckedThrough : PotentialWholeLineCuts) { + const int LineStartPos = LineCheckedThrough.first; + const int CheckedThroughPos = LineCheckedThrough.second; + + int LineEndPos = CheckedThroughPos; + while (LineEndPos < Code.size() && + !isVerticalWhitespace(FileText[LineEndPos])) { + ++LineEndPos; + } + + assert(LineEndPos >= CheckedThroughPos); + StringRef TrailingText(FileText + CheckedThroughPos, + LineEndPos - CheckedThroughPos); + assert(LineEndPos >= LineStartPos); + StringRef OriginalLine(FileText + LineStartPos, LineEndPos - LineStartPos); + if (isWhitespace(TrailingText) && !isWhitespace(OriginalLine)) { + const char *CutTo = skipNewline(FileText + LineEndPos, Code.end()); + int CutCount = CutTo - FileText - LineStartPos; + llvm::Error Err = NewReplaces.add( + tooling::Replacement(FilePath, LineStartPos, CutCount, "")); + if (Err) { + return llvm::Expected(std::move(Err)); + } + } + } + return NewReplaces; +} + llvm::Expected cleanupAroundReplacements(StringRef Code, const tooling::Replacements &Replaces, const FormatStyle &Style) { @@ -2369,7 +2451,12 @@ // Make header insertion replacements insert new headers into correct blocks. tooling::Replacements NewReplaces = fixCppIncludeInsertions(Code, Replaces, Style); - return processReplacements(Cleanup, Code, NewReplaces, Style); + llvm::Expected ProcessedReplaces = + processReplacements(Cleanup, Code, NewReplaces, Style); + // If success, also remove lines made blank by removals. + return (ProcessedReplaces + ? format::removeNewlyBlankLines(Code, *ProcessedReplaces) + : std::move(ProcessedReplaces)); } namespace internal {