Index: clang-tools-extra/clang-tidy/ClangTidy.cpp =================================================================== --- clang-tools-extra/clang-tidy/ClangTidy.cpp +++ clang-tools-extra/clang-tidy/ClangTidy.cpp @@ -21,8 +21,10 @@ #include "ExpandModularHeadersPPCallbacks.h" #include "clang/AST/ASTConsumer.h" #include "clang/AST/ASTContext.h" -#include "clang/AST/Decl.h" +#include "clang/AST/CommentLexer.h" // for skipNewline() +#include "clang/AST/CommentParser.h" // for isWhitespace(StringRef) #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Basic/CharInfo.h" // for isWhiteSpace(char), isVerticalWhitespace(char) #include "clang/Config/config.h" #include "clang/Format/Format.h" #include "clang/Frontend/ASTConsumers.h" @@ -94,6 +96,83 @@ }; #endif // CLANG_ENABLE_STATIC_ANALYZER +/// Examines Replacements for consecutive sequences of one or more +/// Replacements on the same line that would leave a previously non-blank line +/// blank. Replaces each with single Replacement that removes the entire line +/// including trailing newline character(s), to avoid introducing blank lines. +void removeNewlyBlankLinesFromReplacements(StringRef Code, + Replacements &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 unsigned 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) && + isWhitespace(ReplacementText); + LineCheckedThroughPos = R.getOffset() + R.getLength(); + } + + if (LineBlankSoFar) { + PotentialWholeLineCuts.push_back( + std::make_pair(LineStartPos, LineCheckedThroughPos)); + } + + // Actually 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; + Replaces.add(Replacement(FilePath, LineStartPos, CutCount, "")); + } + } +} + class ErrorReporter { public: ErrorReporter(ClangTidyContext &Context, bool ApplyFixes, @@ -216,6 +295,7 @@ llvm::errs() << llvm::toString(FormattedReplacements.takeError()) << ". Skipping formatting.\n"; } + removeNewlyBlankLinesFromReplacements(Code, Replacements.get()); if (!tooling::applyAllReplacements(Replacements.get(), Rewrite)) { llvm::errs() << "Can't apply replacements for file " << File << "\n"; } 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/readability-redundant-control-flow.cpp =================================================================== --- clang-tools-extra/test/clang-tidy/readability-redundant-control-flow.cpp +++ clang-tools-extra/test/clang-tidy/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/readability-redundant-declaration.cpp =================================================================== --- clang-tools-extra/test/clang-tidy/readability-redundant-declaration.cpp +++ clang-tools-extra/test/clang-tidy/readability-redundant-declaration.cpp @@ -77,3 +77,10 @@ extern inline void g(); // extern g // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: redundant 'g' declaration // CHECK-FIXES: {{^}}// extern g{{$}} + +// 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/readability-redundant-member-init.cpp =================================================================== --- clang-tools-extra/test/clang-tidy/readability-redundant-member-init.cpp +++ clang-tools-extra/test/clang-tidy/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/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;