[clang] [Rewrite] Fix offset computation in RemoveText (PR #73827)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 29 09:28:18 PST 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Sebastian Poeplau (sebastianpoeplau)
<details>
<summary>Changes</summary>
When removing a source range from the RewriterBuffer, Clang needs to compute the size of the actual range in the RewriterBuffer (as new elements may have been inserted at any position of the original range). Once this is done, it then maps the original source location offset to real offsets into the RewriteBuffer, supposedly accounting for modifications of the RewriteBuffer (insertion/deletion of text). However, it actually does not account for modifications at the beginning of the range, which is inconsistent behavior as they are taken into account when computing the size of the range. This commit fixes this slightly-off behavior.
---
Full diff: https://github.com/llvm/llvm-project/pull/73827.diff
4 Files Affected:
- (modified) clang/include/clang/Rewrite/Core/RewriteBuffer.h (+1-1)
- (modified) clang/include/clang/Rewrite/Core/Rewriter.h (+6-3)
- (modified) clang/lib/Rewrite/Rewriter.cpp (+22-6)
- (modified) clang/unittests/Rewrite/RewriterTest.cpp (+25)
``````````diff
diff --git a/clang/include/clang/Rewrite/Core/RewriteBuffer.h b/clang/include/clang/Rewrite/Core/RewriteBuffer.h
index b8f34174b715664..295910aa715fc59 100644
--- a/clang/include/clang/Rewrite/Core/RewriteBuffer.h
+++ b/clang/include/clang/Rewrite/Core/RewriteBuffer.h
@@ -58,7 +58,7 @@ class RewriteBuffer {
/// RemoveText - Remove the specified text.
void RemoveText(unsigned OrigOffset, unsigned Size,
- bool removeLineIfEmpty = false);
+ bool removeLineIfEmpty = false, bool AfterInserts = true);
/// InsertText - Insert some text at the specified point, where the offset in
/// the buffer is specified relative to the original SourceBuffer. The
diff --git a/clang/include/clang/Rewrite/Core/Rewriter.h b/clang/include/clang/Rewrite/Core/Rewriter.h
index c89015e4055820c..4cf846d777bd4db 100644
--- a/clang/include/clang/Rewrite/Core/Rewriter.h
+++ b/clang/include/clang/Rewrite/Core/Rewriter.h
@@ -139,17 +139,20 @@ class Rewriter {
/// RemoveText - Remove the specified text region.
bool RemoveText(SourceLocation Start, unsigned Length,
- RewriteOptions opts = RewriteOptions());
+ RewriteOptions opts = RewriteOptions(),
+ bool AfterInserts = true);
/// Remove the specified text region.
bool RemoveText(CharSourceRange range,
RewriteOptions opts = RewriteOptions()) {
- return RemoveText(range.getBegin(), getRangeSize(range, opts), opts);
+ return RemoveText(range.getBegin(), getRangeSize(range, opts), opts,
+ !opts.IncludeInsertsAtBeginOfRange);
}
/// Remove the specified text region.
bool RemoveText(SourceRange range, RewriteOptions opts = RewriteOptions()) {
- return RemoveText(range.getBegin(), getRangeSize(range, opts), opts);
+ return RemoveText(range.getBegin(), getRangeSize(range, opts), opts,
+ !opts.IncludeInsertsAtBeginOfRange);
}
/// ReplaceText - This method replaces a range of characters in the input
diff --git a/clang/lib/Rewrite/Rewriter.cpp b/clang/lib/Rewrite/Rewriter.cpp
index ef2858990dd954e..901e11e8b439e92 100644
--- a/clang/lib/Rewrite/Rewriter.cpp
+++ b/clang/lib/Rewrite/Rewriter.cpp
@@ -55,19 +55,34 @@ static inline bool isWhitespaceExceptNL(unsigned char c) {
}
void RewriteBuffer::RemoveText(unsigned OrigOffset, unsigned Size,
- bool removeLineIfEmpty) {
+ bool removeLineIfEmpty, bool AfterInserts) {
// Nothing to remove, exit early.
if (Size == 0) return;
- unsigned RealOffset = getMappedOffset(OrigOffset, true);
+ unsigned RealOffset = getMappedOffset(OrigOffset, AfterInserts);
assert(RealOffset+Size <= Buffer.size() && "Invalid location");
// Remove the dead characters.
Buffer.erase(RealOffset, Size);
// Add a delta so that future changes are offset correctly.
- AddReplaceDelta(OrigOffset, -Size);
-
+ if (!AfterInserts) {
+ // In this case, we must adjust two deltas:
+ // * The one that corresponds to text inserted at the RealOffset
+ // location. We must thus compute the size of the text inserted there.
+ //
+ // * The one that corresponds to the rest of the range, excluding the
+ // text inserted at the RealOffset location.
+ unsigned OffsetBeforeInserts = RealOffset;
+ unsigned OffsetAfterInserts = getMappedOffset(OrigOffset, true);
+ unsigned InsertedTextSize = OffsetAfterInserts - OffsetBeforeInserts;
+ if (InsertedTextSize != 0)
+ AddInsertDelta(OrigOffset, -InsertedTextSize);
+ if (InsertedTextSize != Size)
+ AddReplaceDelta(OrigOffset, -(Size - InsertedTextSize));
+ } else {
+ AddReplaceDelta(OrigOffset, -Size);
+ }
if (removeLineIfEmpty) {
// Find the line that the remove occurred and if it is completely empty
// remove the line as well.
@@ -303,11 +318,12 @@ bool Rewriter::InsertTextAfterToken(SourceLocation Loc, StringRef Str) {
/// RemoveText - Remove the specified text region.
bool Rewriter::RemoveText(SourceLocation Start, unsigned Length,
- RewriteOptions opts) {
+ RewriteOptions opts, bool AfterInserts) {
if (!isRewritable(Start)) return true;
FileID FID;
unsigned StartOffs = getLocationOffsetAndFileID(Start, FID);
- getEditBuffer(FID).RemoveText(StartOffs, Length, opts.RemoveLineIfEmpty);
+ getEditBuffer(FID).RemoveText(StartOffs, Length, opts.RemoveLineIfEmpty,
+ AfterInserts);
return false;
}
diff --git a/clang/unittests/Rewrite/RewriterTest.cpp b/clang/unittests/Rewrite/RewriterTest.cpp
index ca72dde46fda799..46050eac7ed650e 100644
--- a/clang/unittests/Rewrite/RewriterTest.cpp
+++ b/clang/unittests/Rewrite/RewriterTest.cpp
@@ -77,4 +77,29 @@ TEST(Rewriter, ReplaceTextRangeTypes) {
EXPECT_EQ(T.Rewrite.getRewrittenText(T.makeCharRange(42, 47)), "0;");
}
+TEST(Rewriter, RemoveTextRangeTypes) {
+ // Check that the correct text is removed for CharSourceRange and SourceRange.
+ // Ranges remain in terms of the original text, but include any inserted text
+ // within the range.
+ StringRef Code = "int main(int argc, char *argv[]) { return argc; }";
+ // insert "foo" ^
+ // get ^~~~~ = "fooargc;"
+ // remove char range ^~
+ // get ^~~~~ = "gc;"
+ // insert "bar " ^
+ // get ^~~~~ = "bargc;"
+ // remove source range ^~
+ // get ^~~~~ = ";"
+ RangeTypeTest T(Code, 42, 44);
+ SourceLocation InsertSloc = T.makeLoc(42);
+ T.Rewrite.InsertText(InsertSloc, "foo");
+ T.Rewrite.RemoveText(T.CRange);
+ EXPECT_EQ(T.Rewrite.getRewrittenText(T.makeCharRange(42, 47)), "gc;");
+
+ T.Rewrite.InsertText(InsertSloc, "bar ");
+ EXPECT_EQ(T.Rewrite.getRewrittenText(T.makeCharRange(42, 47)), "bar gc;");
+ T.Rewrite.RemoveText(T.SRange);
+ EXPECT_EQ(T.Rewrite.getRewrittenText(T.makeCharRange(42, 47)), ";");
+}
+
} // anonymous namespace
``````````
</details>
https://github.com/llvm/llvm-project/pull/73827
More information about the cfe-commits
mailing list