poelmanc updated this revision to Diff 224089.
poelmanc retitled this revision from "Clang-tidy fixes should avoid creating
new blank lines" to "Clang-tidy fix removals removing all non-blank text from a
line should remove the line".
poelmanc edited the summary of this revision.
poelmanc added a comment.
Thanks for the prompt review and feedback! MyDeveloperDay, your "drive-by"
comments are quite welcome as the goal is to make clang-tidy and clang-format
work better in tandem. You are correct that my prior version could have
thwarted someone's chang-tidy checker that adds whitespace, which was an
oversight on my part. I've updated the patch to change `isWhitespace(
RemovalText )` to `RemovalText.isBlank()`. Now this only applies to
//Removals// that remove all non-blank text from a line. I also updated the
Title and Summary accordingly, adding the example that most confounded me of ":
a()\n", which readability-redundant-member-init makes blank with a combination
of two separate removals: one for the ":" and one for the "a()".
I understand and appreciate the feedback:
- //this removal of extra white space needs to be handled at the point the
replacement is created and not on all the final replacements where the context
is lost//
- //Checks that edit the source code should be improved to delete newlines
where they become unnecessary//
Indeed I first tried fixing this entirely within
readability-redundant-member-init. One problem was that when a line becomes
blank due to two separate removals, code that peeks ahead or backwards to see
if the line is blank doesn't know about other removals that will occur. Perhaps
this can be fixed if the check() function has access to other removals that
have already been created? (See also //Note 1//.) As I broadened my search I
found code in readability-redundant-control-flow that heuristically tried to
remove blank lines, but it failed to examine the entire line and thus removed
newlines from the ends of lines that were not actually blank. (See the test
case I added to readability-redundant-control-flow.cpp, where ` /* NOTE */
continue;\n` had the newline following `continue;` removed, leaving the comment
attached to the next line.) And I found that readability-redundant-declaration
did not attempt to remove blank lines, and its test cases specifically added
comments to all removed declarations so that in its test cases, none of the
resulting lines were blank. (See the test case I added to
readability-redundant-declaration.cpp.)
Digging into these examples led me to the exact opposite conclusion: that the
"newly blank line" problem needs to be solved consistently at a higher-level
where all removals can be examined jointly, rather than trying to address it
within each check.
I guess here's the high-level question: //should all removals that remove all
non-blank text from a line also delete the line//?
- If //yes//: consider reviewing the patch
- If //mostly//: consider adding this as a top-level clang-tidy option that's
off by default
- //Otherwise//: if it's possible to address this solely within each of
readability-redundant-member-init, readability-redundant-control-flow,
readability-redundant-declaration, etc., I can revisit that original approach.
I'd appreciate pointers on how to access prior Removals on the same line from
within check(), and any guesses regarding the Windows line-ending issue of
//Note 1//.
Thanks!
//Note 1:// When applied to a file with Windows-style 2-character line endings,
adding a Removal to readability-redundant-member-init whose Length was set to
include the two-character newline would delete only the first character of the
newline, leaving a \r. Then when I increased the removal length by 1, it would
delete both newline characters //and// the first character of the line
//after// the newline, which would be extremely undesirable. Updating removals
at the top-level of ClangTidy.cpp did not have this problem. I was never able
to track down where/why this error occurred, though it's surely fixable.
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/ClangTidy.cpp
clang-tools-extra/clang-tidy/readability/RedundantControlFlowCheck.cpp
clang-tools-extra/test/clang-tidy/readability-redundant-control-flow.cpp
clang-tools-extra/test/clang-tidy/readability-redundant-declaration.cpp
clang-tools-extra/test/clang-tidy/readability-redundant-member-init.cpp
clang/include/clang/AST/CommentLexer.h
clang/include/clang/AST/CommentParser.h
clang/lib/AST/CommentLexer.cpp
clang/lib/AST/CommentParser.cpp
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/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/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-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-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 <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);
}
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<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 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) &&
+ ReplacementText.empty();
+ 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";
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
