[PATCH] D61466: [Rewrite][NFC] Add FIXME about RemoveLineIfEmpty

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: akyrtzi, Eugene.Zelenko, rsmith.
Herald added a subscriber: dexonsmith.
Herald added a project: clang.

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.

While I'm sure of the bug, some of these comments might be naive as I haven't fully digested the implementation here.  If someone prefers to write entirely different comments or wants to fix the bug, I'm happy to abandon this patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61466

Files:
  clang/include/clang/Rewrite/Core/Rewriter.h
  clang/lib/Rewrite/Rewriter.cpp


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.  Even if that's
+      // corrected, this still assumes all the whitespace characters being
+      // removed were originally contiguous.  Is that OK?
       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,16 @@
 
     /// 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.  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() {}


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D61466.197856.patch
Type: text/x-patch
Size: 2138 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190502/7244f244/attachment.bin>