diff options
author | Joel E. Denny <jdenny.ornl@gmail.com> | 2019-08-15 21:17:48 +0000 |
---|---|---|
committer | Joel E. Denny <jdenny.ornl@gmail.com> | 2019-08-15 21:17:48 +0000 |
commit | 9be6d7edb20b87dfa35d21e981591a9a81959344 (patch) | |
tree | f204ab31e62a6e7a66e8f2493d1c8b7c3c2bf313 | |
parent | be8a2f75657b27ea113517c3279f0489a7f4018c (diff) | |
download | llvm-9be6d7edb20b87dfa35d21e981591a9a81959344.zip llvm-9be6d7edb20b87dfa35d21e981591a9a81959344.tar.gz llvm-9be6d7edb20b87dfa35d21e981591a9a81959344.tar.bz2 |
[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
llvm-svn: 369049
-rw-r--r-- | clang/include/clang/Rewrite/Core/Rewriter.h | 11 | ||||
-rw-r--r-- | clang/lib/Rewrite/Rewriter.cpp | 11 | ||||
-rw-r--r-- | clang/unittests/Rewrite/RewriteBufferTest.cpp | 73 |
3 files changed, 90 insertions, 5 deletions
diff --git a/clang/include/clang/Rewrite/Core/Rewriter.h b/clang/include/clang/Rewrite/Core/Rewriter.h index 84c5ac3..c89015e 100644 --- a/clang/include/clang/Rewrite/Core/Rewriter.h +++ b/clang/include/clang/Rewrite/Core/Rewriter.h @@ -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() {} diff --git a/clang/lib/Rewrite/Rewriter.cpp b/clang/lib/Rewrite/Rewriter.cpp index 881399e..33718b7 100644 --- a/clang/lib/Rewrite/Rewriter.cpp +++ b/clang/lib/Rewrite/Rewriter.cpp @@ -96,6 +96,17 @@ void RewriteBuffer::RemoveText(unsigned OrigOffset, unsigned Size, } 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'*/)); } } diff --git a/clang/unittests/Rewrite/RewriteBufferTest.cpp b/clang/unittests/Rewrite/RewriteBufferTest.cpp index eb8d986..bf29c0f 100644 --- a/clang/unittests/Rewrite/RewriteBufferTest.cpp +++ b/clang/unittests/Rewrite/RewriteBufferTest.cpp @@ -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 |