poelmanc updated this revision to Diff 228558.
poelmanc added a comment.
I just rebased this patch on the latest master. I believe I've addressed all
the comments raised so far. Should I add mention of this change to the release
notes?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68682/new/
https://reviews.llvm.org/D68682
Files:
clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-control-flow.cpp
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-declaration.cpp
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-member-init.cpp
clang/include/clang/AST/CommentLexer.h
clang/include/clang/AST/CommentParser.h
clang/include/clang/Format/Format.h
clang/lib/AST/CommentLexer.cpp
clang/lib/AST/CommentParser.cpp
clang/lib/Format/Format.cpp
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<tooling::Replacements>
+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<std::pair<int, int>> 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<tooling::Replacements>(std::move(Err));
+ }
+ }
+ }
+ return NewReplaces;
+}
+
llvm::Expected<tooling::Replacements>
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<tooling::Replacements> 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 {
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/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/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<tooling::Replacements>
+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/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/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-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-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-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 <typename T>
+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/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<Stmt>(*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);
}
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
