jdenny updated this revision to Diff 208795.
jdenny retitled this revision from "[Rewrite][NFC] Add FIXME about
RemoveLineIfEmpty" to "[Rewrite][NFC] Add FIXMEs and tests for
RemoveLineIfEmpty bug".
jdenny edited the summary of this revision.
jdenny added a comment.
Rebase, and add a unit test to demonstrate the bug.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61466/new/
https://reviews.llvm.org/D61466
Files:
clang/include/clang/Rewrite/Core/Rewriter.h
clang/lib/Rewrite/Rewriter.cpp
clang/unittests/Rewrite/RewriteBufferTest.cpp
Index: clang/unittests/Rewrite/RewriteBufferTest.cpp
===================================================================
--- clang/unittests/Rewrite/RewriteBufferTest.cpp
+++ clang/unittests/Rewrite/RewriteBufferTest.cpp
@@ -14,6 +14,16 @@
namespace {
+#define EXPECT_OUTPUT(Buf, Output) EXPECT_EQ(Output, writeOutput(Buf))
+
+static std::string writeOutput(const RewriteBuffer &Buf) {
+ std::string Result;
+ raw_string_ostream OS(Result);
+ Buf.write(OS);
+ OS.flush();
+ return Result;
+}
+
static void tagRange(unsigned Offset, unsigned Len, StringRef tagName,
RewriteBuffer &Buf) {
std::string BeginTag;
@@ -40,11 +50,64 @@
tagRange(Pos, TagStr.size(), "outer", Buf);
tagRange(Pos, TagStr.size(), "inner", Buf);
- std::string Result;
- raw_string_ostream OS(Result);
- Buf.write(OS);
- OS.flush();
- EXPECT_EQ(Output, Result);
+ EXPECT_OUTPUT(Buf, Output);
+}
+
+TEST(RewriteBuffer, DISABLED_RemoveLineIfEmpty_XFAIL) {
+ StringRef Input = "def\n"
+ "ghi\n"
+ "jkl\n";
+ RewriteBuffer Buf;
+ Buf.Initialize(Input);
+
+ // Insert "abc\n" at the start.
+ Buf.InsertText(0, "abc\n");
+ EXPECT_OUTPUT(Buf, "abc\n"
+ "def\n"
+ "ghi\n"
+ "jkl\n");
+
+ // Remove "def\n".
+ //
+ // After the removal of "def", we have:
+ //
+ // "abc\n"
+ // "\n"
+ // "ghi\n"
+ // "jkl\n"
+ //
+ // Because removeLineIfEmpty=true, RemoveText has to remove the "\n" left on
+ // the line. This happens correctly for the rewrite buffer itself, so the
+ // next check below passes.
+ //
+ // However, RemoveText's implementation incorrectly records the delta for
+ // removing the "\n" using the rewrite buffer offset, 4, where it was
+ // supposed to use the original input offset, 3. Interpreted as an original
+ // input offset, 4 points to "g" not to "\n". Thus, any future modifications
+ // at the original input's "g" will incorrectly see "g" as having become an
+ // empty string and so will map to the next character, "h", in the rewrite
+ // buffer.
+ StringRef RemoveStr0 = "def";
+ Buf.RemoveText(Input.find(RemoveStr0), RemoveStr0.size(),
+ /*removeLineIfEmpty*/ true);
+ EXPECT_OUTPUT(Buf, "abc\n"
+ "ghi\n"
+ "jkl\n");
+
+ // Try to remove "ghi\n".
+ //
+ // As discussed above, the original input offset for "ghi\n" incorrectly
+ // maps to the rewrite buffer offset for "hi\nj", so we end up with:
+ //
+ // "abc\n"
+ // "gkl\n"
+ //
+ // To show that removeLineIfEmpty=true is the culprit, change true to false
+ // and append a newline to RemoveStr0 above. The test then passes.
+ StringRef RemoveStr1 = "ghi\n";
+ Buf.RemoveText(Input.find(RemoveStr1), RemoveStr1.size());
+ EXPECT_OUTPUT(Buf, "abc\n"
+ "jkl\n");
}
} // anonymous namespace
Index: clang/lib/Rewrite/Rewriter.cpp
===================================================================
--- clang/lib/Rewrite/Rewriter.cpp
+++ clang/lib/Rewrite/Rewriter.cpp
@@ -96,6 +96,15 @@
}
if (posI != end() && *posI == '\n') {
Buffer.erase(curLineStartOffs, lineSize + 1/* + '\n'*/);
+ // FIXME: Here, the offset of the start of the line is supposed to be
+ // expressed in terms of the original input not the "real" rewrite
+ // buffer. How do we compute that reliably? It might be tempting to use
+ // curLineStartOffs + OrigOffset - RealOffset, but that assumes the
+ // difference between the original and real offset is the same at the
+ // removed text and at the start of the line, but that's not true if
+ // edits were previously made earlier on the line. This bug is also
+ // documented by a FIXME on the definition of
+ // clang::Rewriter::RewriteOptions::RemoveLineIfEmpty.
AddReplaceDelta(curLineStartOffs, -(lineSize + 1/* + '\n'*/));
}
}
Index: clang/include/clang/Rewrite/Core/Rewriter.h
===================================================================
--- clang/include/clang/Rewrite/Core/Rewriter.h
+++ clang/include/clang/Rewrite/Core/Rewriter.h
@@ -46,6 +46,17 @@
/// If true and removing some text leaves a blank line
/// also remove the empty line (false by default).
+ ///
+ /// FIXME: This sometimes corrupts the file's rewrite buffer due to
+ /// incorrect indexing in the implementation (see the FIXME in
+ /// clang::RewriteBuffer::RemoveText). Moreover, it's inefficient because
+ /// it must scan the buffer from the beginning to find the start of the
+ /// line. When feasible, it's better for the caller to check for a blank
+ /// line and then, if found, expand the removal range to include it.
+ /// Checking for a blank line is easy if, for example, the caller can
+ /// guarantee this is the first edit of a line. In that case, it can just
+ /// scan before and after the removal range until the next newline or
+ /// begin/end of the input.
bool RemoveLineIfEmpty = false;
RewriteOptions() {}
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
