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/Basic/CharInfo.h =================================================================== --- clang/include/clang/Basic/CharInfo.h +++ clang/include/clang/Basic/CharInfo.h @@ -193,6 +193,34 @@ return true; } +/// 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. +LLVM_READONLY inline const char *skipNewlineChars(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; +} + +/// Returns true if and only if S consists entirely of whitespace. +LLVM_READONLY inline bool isWhitespaceStringRef(llvm::StringRef S) { + for (StringRef::const_iterator I = S.begin(), E = S.end(); I != E; ++I) { + if (!isWhitespace(*I)) + return false; + } + return true; +} + } // end namespace clang #endif 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 @@ -131,21 +131,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) { @@ -254,7 +239,7 @@ (EscapePtr - 2 >= BufferPtr && EscapePtr[0] == '/' && EscapePtr[-1] == '?' && EscapePtr[-2] == '?')) { // We found an escaped newline. - CurPtr = skipNewline(CurPtr, BufferEnd); + CurPtr = skipNewlineChars(CurPtr, BufferEnd); } else return CurPtr; // Not an escaped newline. } @@ -302,7 +287,7 @@ switch (*TokenPtr) { case '\n': case '\r': - TokenPtr = skipNewline(TokenPtr, CommentEnd); + TokenPtr = skipNewlineChars(TokenPtr, CommentEnd); formTokenWithChars(T, TokenPtr, tok::newline); if (CommentState == LCS_InsideCComment) @@ -477,7 +462,7 @@ // text content. if (BufferPtr != CommentEnd && isVerticalWhitespace(*BufferPtr)) { - BufferPtr = skipNewline(BufferPtr, CommentEnd); + BufferPtr = skipNewlineChars(BufferPtr, CommentEnd); State = LS_VerbatimBlockBody; return; } @@ -503,7 +488,7 @@ if (Pos == StringRef::npos) { // Current line is completely verbatim. TextEnd = Newline; - NextLine = skipNewline(Newline, CommentEnd); + NextLine = skipNewlineChars(Newline, CommentEnd); } else if (Pos == 0) { // Current line contains just an end command. const char *End = BufferPtr + VerbatimBlockEndCommandName.size(); Index: clang/lib/AST/CommentParser.cpp =================================================================== --- clang/lib/AST/CommentParser.cpp +++ clang/lib/AST/CommentParser.cpp @@ -16,14 +16,6 @@ namespace clang { -static inline bool isWhitespace(llvm::StringRef S) { - for (StringRef::const_iterator I = S.begin(), E = S.end(); I != E; ++I) { - if (!isWhitespace(*I)) - return false; - } - return true; -} - namespace comments { /// Re-lexes a sequence of tok::text tokens. @@ -609,7 +601,7 @@ } // Also allow [tok::newline, tok::text, tok::newline] if the middle // tok::text is just whitespace. - if (Tok.is(tok::text) && isWhitespace(Tok.getText())) { + if (Tok.is(tok::text) && isWhitespaceStringRef(Tok.getText())) { Token WhitespaceTok = Tok; consumeToken(); if (Tok.is(tok::newline) || Tok.is(tok::eof)) { Index: clang/lib/Format/Format.cpp =================================================================== --- clang/lib/Format/Format.cpp +++ clang/lib/Format/Format.cpp @@ -25,6 +25,7 @@ #include "UnwrappedLineParser.h" #include "UsingDeclarationsSorter.h" #include "WhitespaceManager.h" +#include "clang/Basic/CharInfo.h" #include "clang/Basic/Diagnostic.h" #include "clang/Basic/DiagnosticOptions.h" #include "clang/Basic/SourceManager.h" @@ -2356,6 +2357,87 @@ } // 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::vector> 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 && + isWhitespaceStringRef(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 (isWhitespaceStringRef(TrailingText) && + !isWhitespaceStringRef(OriginalLine)) { + const char *CutTo = skipNewlineChars(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 {