Page MenuHomePhabricator
Files F11064754

D61466.diff
No OneTemporary
Actions

File Metadata

Created
Sun, Dec 15, 2:33 AM

D61466.diff
View Options

Index: cfe/trunk/include/clang/Rewrite/Core/Rewriter.h
===================================================================
--- cfe/trunk/include/clang/Rewrite/Core/Rewriter.h
+++ cfe/trunk/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() {}
Index: cfe/trunk/lib/Rewrite/Rewriter.cpp
===================================================================
--- cfe/trunk/lib/Rewrite/Rewriter.cpp
+++ cfe/trunk/lib/Rewrite/Rewriter.cpp
@@ -96,6 +96,17 @@
}
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. A reproducer for
+ // the implementation below is the test RemoveLineIfEmpty in
+ // clang/unittests/Rewrite/RewriteBufferTest.cpp.
AddReplaceDelta(curLineStartOffs, -(lineSize + 1/* + '\n'*/));
}
}
Index: cfe/trunk/unittests/Rewrite/RewriteBufferTest.cpp
===================================================================
--- cfe/trunk/unittests/Rewrite/RewriteBufferTest.cpp
+++ cfe/trunk/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

Event Timeline