Merge conflicting replacements when they are order-independent.
Summary:
Now two replacements are considered order-independent if applying them in
either order produces the same result. These include (but not restricted
to) replacements that:
- don't overlap (being directly adjacent is fine) and
- are overlapping deletions.
- are insertions at the same offset and applying them in either order
has the same effect, i.e. X + Y = Y + X if one inserts text X and the
other inserts text Y.
Discussion about this design can be found in D24717
Reviewers: djasper, klimek
Subscribers: omtcyfz, cfe-commits
Differential Revision: https://reviews.llvm.org/D24800
llvm-svn: 282577
diff --git a/clang/unittests/Tooling/RefactoringTest.cpp b/clang/unittests/Tooling/RefactoringTest.cpp
index b4c269d..0a70a11 100644
--- a/clang/unittests/Tooling/RefactoringTest.cpp
+++ b/clang/unittests/Tooling/RefactoringTest.cpp
@@ -101,18 +101,56 @@
TEST_F(ReplacementTest, FailAddReplacements) {
Replacements Replaces;
- auto Err = Replaces.add(Replacement("x.cc", 0, 10, "3"));
+ Replacement Deletion("x.cc", 0, 10, "3");
+ auto Err = Replaces.add(Deletion);
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
- Err = Replaces.add(Replacement("x.cc", 0, 2, ""));
+ Err = Replaces.add(Replacement("x.cc", 0, 2, "a"));
EXPECT_TRUE((bool)Err);
llvm::consumeError(std::move(Err));
- Err = Replaces.add(Replacement("x.cc", 2, 2, ""));
+ Err = Replaces.add(Replacement("x.cc", 2, 2, "a"));
EXPECT_TRUE((bool)Err);
llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement("y.cc", 20, 2, ""));
EXPECT_TRUE((bool)Err);
llvm::consumeError(std::move(Err));
+ EXPECT_EQ(1u, Replaces.size());
+ EXPECT_EQ(Deletion, *Replaces.begin());
+}
+
+TEST_F(ReplacementTest, DeletionInReplacements) {
+ Replacements Replaces;
+ Replacement R("x.cc", 0, 10, "3");
+ auto Err = Replaces.add(R);
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+ Err = Replaces.add(Replacement("x.cc", 0, 2, ""));
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+ Err = Replaces.add(Replacement("x.cc", 2, 2, ""));
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+ EXPECT_EQ(1u, Replaces.size());
+ EXPECT_EQ(R, *Replaces.begin());
+}
+
+TEST_F(ReplacementTest, OverlappingReplacements) {
+ Replacements Replaces;
+ auto Err = Replaces.add(Replacement("x.cc", 0, 3, "345"));
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+ Err = Replaces.add(Replacement("x.cc", 2, 3, "543"));
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ EXPECT_EQ(1u, Replaces.size());
+ EXPECT_EQ(Replacement("x.cc", 0, 5, "34543"), *Replaces.begin());
+
+ Err = Replaces.add(Replacement("x.cc", 2, 1, "5"));
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+ EXPECT_EQ(1u, Replaces.size());
+ EXPECT_EQ(Replacement("x.cc", 0, 5, "34543"), *Replaces.begin());
}
TEST_F(ReplacementTest, AddAdjacentInsertionAndReplacement) {
@@ -137,6 +175,116 @@
EXPECT_EQ(Replaces.size(), 2u);
}
+TEST_F(ReplacementTest, MergeNewDeletions) {
+ Replacements Replaces;
+ Replacement ContainingReplacement("x.cc", 0, 10, "");
+ auto Err = Replaces.add(ContainingReplacement);
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ Err = Replaces.add(Replacement("x.cc", 5, 3, ""));
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ Err = Replaces.add(Replacement("x.cc", 0, 10, ""));
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ Err = Replaces.add(Replacement("x.cc", 5, 5, ""));
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ EXPECT_EQ(1u, Replaces.size());
+ EXPECT_EQ(*Replaces.begin(), ContainingReplacement);
+}
+
+TEST_F(ReplacementTest, MergeOverlappingButNotAdjacentReplacement) {
+ Replacements Replaces;
+ auto Err = Replaces.add(Replacement("x.cc", 0, 2, ""));
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ Err = Replaces.add(Replacement("x.cc", 5, 5, ""));
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ Replacement After = Replacement("x.cc", 10, 5, "");
+ Err = Replaces.add(After);
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ Replacement ContainingReplacement("x.cc", 0, 10, "");
+ Err = Replaces.add(ContainingReplacement);
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ EXPECT_EQ(2u, Replaces.size());
+ EXPECT_EQ(*Replaces.begin(), ContainingReplacement);
+ EXPECT_EQ(*(++Replaces.begin()), After);
+}
+
+TEST_F(ReplacementTest, InsertionBeforeMergedDeletions) {
+ Replacements Replaces;
+
+ Replacement Insertion("x.cc", 0, 0, "123");
+ auto Err = Replaces.add(Insertion);
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ Err = Replaces.add(Replacement("x.cc", 5, 5, ""));
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ Replacement Deletion("x.cc", 0, 10, "");
+ Err = Replaces.add(Deletion);
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ EXPECT_EQ(2u, Replaces.size());
+ EXPECT_EQ(*Replaces.begin(), Insertion);
+ EXPECT_EQ(*(++Replaces.begin()), Deletion);
+}
+
+TEST_F(ReplacementTest, MergeOverlappingDeletions) {
+ Replacements Replaces;
+ auto Err = Replaces.add(Replacement("x.cc", 0, 2, ""));
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ Err = Replaces.add(Replacement("x.cc", 0, 5, ""));
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ EXPECT_EQ(1u, Replaces.size());
+ EXPECT_EQ(Replacement("x.cc", 0, 5, ""), *Replaces.begin());
+
+ Err = Replaces.add(Replacement("x.cc", 1, 5, ""));
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+ EXPECT_EQ(1u, Replaces.size());
+ EXPECT_EQ(Replacement("x.cc", 0, 6, ""), *Replaces.begin());
+}
+
+TEST_F(ReplacementTest, FailedMergeExistingDeletions) {
+ Replacements Replaces;
+ Replacement First("x.cc", 0, 2, "");
+ auto Err = Replaces.add(First);
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ Replacement Second("x.cc", 5, 5, "");
+ Err = Replaces.add(Second);
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ Err = Replaces.add(Replacement("x.cc", 1, 10, ""));
+ EXPECT_TRUE(!Err);
+ llvm::consumeError(std::move(Err));
+
+ EXPECT_EQ(1u, Replaces.size());
+ EXPECT_EQ(Replacement("x.cc", 0, 11, ""), *Replaces.begin());
+}
+
TEST_F(ReplacementTest, FailAddRegression) {
Replacements Replaces;
// Create two replacements, where the second one is an insertion of the empty
@@ -155,7 +303,7 @@
llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement("x.cc", 10, 0, ""));
- EXPECT_TRUE((bool)Err);
+ EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
}
@@ -179,7 +327,7 @@
EXPECT_EQ(Replaces.size(), 2u);
}
-TEST_F(ReplacementTest, FailAddInsertAtOtherInsert) {
+TEST_F(ReplacementTest, AddInsertAtOtherInsertWhenOderIndependent) {
Replacements Replaces;
auto Err = Replaces.add(Replacement("x.cc", 10, 0, "a"));
EXPECT_TRUE(!Err);
@@ -189,12 +337,14 @@
llvm::consumeError(std::move(Err));
Replaces.clear();
- Err = Replaces.add(Replacement("x.cc", 10, 0, ""));
+ Err = Replaces.add(Replacement("x.cc", 10, 0, "a"));
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
- Err = Replaces.add(Replacement("x.cc", 10, 0, ""));
- EXPECT_TRUE((bool)Err);
+ Err = Replaces.add(Replacement("x.cc", 10, 0, "aa"));
+ EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
+ EXPECT_EQ(1u, Replaces.size());
+ EXPECT_EQ(Replacement("x.cc", 10, 0, "aaa"), *Replaces.begin());
Replaces.clear();
Err = Replaces.add(Replacement("x.cc", 10, 0, ""));
@@ -204,8 +354,11 @@
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement("x.cc", 10, 0, ""));
- EXPECT_TRUE((bool)Err);
+ EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
+ EXPECT_EQ(2u, Replaces.size());
+ EXPECT_EQ(Replacement("x.cc", 10, 0, ""), *Replaces.begin());
+ EXPECT_EQ(Replacement("x.cc", 10, 3, ""), *std::next(Replaces.begin()));
}
TEST_F(ReplacementTest, InsertBetweenAdjacentReplacements) {
@@ -256,7 +409,7 @@
EXPECT_EQ("xy", Context.getRewrittenText(ID));
}
-TEST_F(ReplacementTest, SkipsDuplicateReplacements) {
+TEST_F(ReplacementTest, AddDuplicateReplacements) {
FileID ID = Context.createInMemoryFile("input.cpp",
"line1\nline2\nline3\nline4");
auto Replaces = toReplacements({Replacement(
@@ -264,18 +417,33 @@
auto Err = Replaces.add(Replacement(
Context.Sources, Context.getLocation(ID, 2, 1), 5, "replaced"));
- EXPECT_TRUE((bool)Err);
+ EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement(Context.Sources, Context.getLocation(ID, 2, 1),
5, "replaced"));
- EXPECT_TRUE((bool)Err);
+ EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
EXPECT_TRUE(applyAllReplacements(Replaces, Context.Rewrite));
EXPECT_EQ("line1\nreplaced\nline3\nline4", Context.getRewrittenText(ID));
}
+TEST_F(ReplacementTest, FailOrderDependentReplacements) {
+ FileID ID = Context.createInMemoryFile("input.cpp",
+ "line1\nline2\nline3\nline4");
+ auto Replaces = toReplacements({Replacement(
+ Context.Sources, Context.getLocation(ID, 2, 1), 5, "other")});
+
+ auto Err = Replaces.add(Replacement(
+ Context.Sources, Context.getLocation(ID, 2, 1), 5, "rehto"));
+ EXPECT_TRUE((bool)Err);
+ llvm::consumeError(std::move(Err));
+
+ EXPECT_TRUE(applyAllReplacements(Replaces, Context.Rewrite));
+ EXPECT_EQ("line1\nother\nline3\nline4", Context.getRewrittenText(ID));
+}
+
TEST_F(ReplacementTest, InvalidSourceLocationFailsApplyAll) {
Replacements Replaces =
toReplacements({Replacement(Context.Sources, SourceLocation(), 5, "2")});