r369049 - [Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug
Joel E. Denny via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 15 14:17:48 PDT 2019
Author: jdenny
Date: Thu Aug 15 14:17:48 2019
New Revision: 369049
URL: http://llvm.org/viewvc/llvm-project?rev=369049&view=rev
Log:
[Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug
I'd like to add these comments to warn others of problems I
encountered when trying to use `RemoveLineIfEmpty`. I originally
tried to fix the problem, but I realized I could implement the
functionality more easily and efficiently in my calling code where I
can make the simplifying assumption that there are no prior edits to
the line from which text is being removed. While I've lost the
motivation to write a fix, which doesn't look easy, I figure a warning
to others is better than silence.
I've added a unit test to demonstrate the problem. I don't know how
to mark it as an expected failure, so I just marked it disabled.
Reviewed By: jkorous
Differential Revision: https://reviews.llvm.org/D61466
Modified:
cfe/trunk/include/clang/Rewrite/Core/Rewriter.h
cfe/trunk/lib/Rewrite/Rewriter.cpp
cfe/trunk/unittests/Rewrite/RewriteBufferTest.cpp
Modified: cfe/trunk/include/clang/Rewrite/Core/Rewriter.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Rewrite/Core/Rewriter.h?rev=369049&r1=369048&r2=369049&view=diff
==============================================================================
--- cfe/trunk/include/clang/Rewrite/Core/Rewriter.h (original)
+++ cfe/trunk/include/clang/Rewrite/Core/Rewriter.h Thu Aug 15 14:17:48 2019
@@ -46,6 +46,17 @@ public:
/// 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() {}
Modified: cfe/trunk/lib/Rewrite/Rewriter.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Rewrite/Rewriter.cpp?rev=369049&r1=369048&r2=369049&view=diff
==============================================================================
--- cfe/trunk/lib/Rewrite/Rewriter.cpp (original)
+++ cfe/trunk/lib/Rewrite/Rewriter.cpp Thu Aug 15 14:17:48 2019
@@ -96,6 +96,17 @@ void RewriteBuffer::RemoveText(unsigned
}
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'*/));
}
}
Modified: cfe/trunk/unittests/Rewrite/RewriteBufferTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Rewrite/RewriteBufferTest.cpp?rev=369049&r1=369048&r2=369049&view=diff
==============================================================================
--- cfe/trunk/unittests/Rewrite/RewriteBufferTest.cpp (original)
+++ cfe/trunk/unittests/Rewrite/RewriteBufferTest.cpp Thu Aug 15 14:17:48 2019
@@ -14,6 +14,16 @@ using namespace clang;
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 @@ TEST(RewriteBuffer, TagRanges) {
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
More information about the cfe-commits
mailing list