Re: [PATCH] D24800: Merge conflicting replacements when they are order-independent.
alexshap added inline comments.
Comment at: cfe/trunk/lib/Tooling/Core/Replacement.cpp:182
@@ +181,3 @@
+llvm::Expected
+Replacements::mergeIfOrderIndependent(const Replacement ) const {
+ Replacements Rs(R);
ioeric wrote:
> alexshap wrote:
> > sorry, probably i am late to the party,
> > however i'd like to ask how this method is supposed to be used by the
> > customers of this code.
> > If i'm not mistaken we have some concrete examples in clang-extra-tools:
> > for instance @omtcyfz referred to this diff on
> > https://reviews.llvm.org/D24914.
> > Basically after this diff the class Replacements contains several methods
> > (add, mergeIfOrderIndependent etc), but mergeIfOrderIndependent potentially
> > will copy a lot of replacements (i don't know if the perf is a real issue
> > here, but at least it looks a bit strange to me).
> > Another question - how are the customers of this code supposed to use the
> > method add ? still suppress the error (like what was happening on
> > https://reviews.llvm.org/D24914) ?
> > Am i missing smth ?
> > cc: @klimek, @djasper.
> >
> First of all, this diff doesn't introduce new interfaces - they are just
> private helper functions, which are mostly called on conflicting replacements
> set with relatively small size.
>
> Secondly, `add` now supports merging order-independent replacements, so it
> handles deduplication to some extend, i.e. identical non-insertion
> replacements can be deduplicated, which happens to solve the issue with
> clang-rename as mentioned in D24914.
>
i see, thank you for the explanation.
Repository:
rL LLVM
https://reviews.llvm.org/D24800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24800: Merge conflicting replacements when they are order-independent.
alexshap added a subscriber: alexshap.
Comment at: cfe/trunk/lib/Tooling/Core/Replacement.cpp:182
@@ +181,3 @@
+llvm::Expected
+Replacements::mergeIfOrderIndependent(const Replacement ) const {
+ Replacements Rs(R);
sorry, probably i am late to the party,
however i'd like to ask how this method is supposed to be used by the customers
of this code.
If i'm not mistaken we have some concrete examples in clang-extra-tools: for
instance @omtcyfz referred to this diff on https://reviews.llvm.org/D24914.
Basically after this diff the class Replacements contains several methods (add,
mergeIfOrderIndependent etc), but mergeIfOrderIndependent potentially will copy
a lot of replacements (i don't know if the perf is a real issue here, but at
least it looks a bit strange to me).
Another question - how are the customers of this code supposed to use the
method add ? still suppress the error (like what was happening on
https://reviews.llvm.org/D24914) ?
Am i missing smth ?
cc: @klimek, @djasper.
Repository:
rL LLVM
https://reviews.llvm.org/D24800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24800: Merge conflicting replacements when they are order-independent.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL282577: Merge conflicting replacements when they are
order-independent. (authored by ioeric).
Changed prior to commit:
https://reviews.llvm.org/D24800?vs=72666=72799#toc
Repository:
rL LLVM
https://reviews.llvm.org/D24800
Files:
cfe/trunk/include/clang/Tooling/Core/Replacement.h
cfe/trunk/lib/Tooling/Core/Replacement.cpp
cfe/trunk/unittests/Tooling/RefactoringTest.cpp
Index: cfe/trunk/include/clang/Tooling/Core/Replacement.h
===
--- cfe/trunk/include/clang/Tooling/Core/Replacement.h
+++ cfe/trunk/include/clang/Tooling/Core/Replacement.h
@@ -167,10 +167,25 @@
/// order-dependent replacements. To control the order in which
/// order-dependent replacements are applied, use merge({R}) with R referring
/// to the changed code after applying all existing replacements.
- /// Two replacements are considered order-independent if they:
+ /// Two replacements A and B are considered order-independent if applying them
+ /// in either order produces the same result. Note that the range of the
+ /// replacement that is applied later still refers to the original code.
+ /// These include (but not restricted to) replacements that:
/// - don't overlap (being directly adjacent is fine) and
- /// - aren't both inserts at the same code location (would be
- /// order-dependent).
+ /// - 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 when inserting X and Y
+ /// respectively.
+ /// Examples:
+ /// 1. Replacement A(0, 0, "a") and B(0, 0, "aa") are order-independent since
+ ///applying them in either order gives replacement (0, 0, "aaa").
+ ///However, A(0, 0, "a") and B(0, 0, "b") are order-dependent since
+ ///applying A first gives (0, 0, "ab") while applying B first gives (B, A,
+ ///"ba").
+ /// 2. Replacement A(0, 2, "123") and B(0, 2, "123") are order-independent
+ ///since applying them in either order gives (0, 2, "123").
+ /// 3. Replacement A(0, 3, "123") and B(2, 3, "321") are order-independent
+ ///since either order gives (0, 5, "12321").
/// Replacements with offset UINT_MAX are special - we do not detect conflicts
/// for such replacements since users may add them intentionally as a special
/// category of replacements.
@@ -213,6 +228,22 @@
Replacements mergeReplacements(const ReplacementsImpl ) const;
+ // Returns `R` with new range that refers to code after `Replaces` being
+ // applied.
+ Replacement getReplacementInChangedCode(const Replacement ) const;
+
+ // Returns a set of replacements that is equivalent to the current
+ // replacements by merging all adjacent replacements. Two sets of replacements
+ // are considered equivalent if they have the same effect when they are
+ // applied.
+ Replacements getCanonicalReplacements() const;
+
+ // If `R` and all existing replacements are order-indepedent, then merge it
+ // with `Replaces` and returns the merged replacements; otherwise, returns an
+ // error.
+ llvm::Expected
+ mergeIfOrderIndependent(const Replacement ) const;
+
ReplacementsImpl Replaces;
};
Index: cfe/trunk/lib/Tooling/Core/Replacement.cpp
===
--- cfe/trunk/lib/Tooling/Core/Replacement.cpp
+++ cfe/trunk/lib/Tooling/Core/Replacement.cpp
@@ -134,14 +134,75 @@
ReplacementText);
}
-llvm::Error makeConflictReplacementsError(const Replacement ,
- const Replacement ) {
+Replacement
+Replacements::getReplacementInChangedCode(const Replacement ) const {
+ unsigned NewStart = getShiftedCodePosition(R.getOffset());
+ unsigned NewEnd = getShiftedCodePosition(R.getOffset() + R.getLength());
+ return Replacement(R.getFilePath(), NewStart, NewEnd - NewStart,
+ R.getReplacementText());
+}
+
+static llvm::Error makeConflictReplacementsError(const Replacement ,
+ const Replacement ) {
return llvm::make_error(
"New replacement:\n" + New.toString() +
"\nconflicts with existing replacement:\n" + Existing.toString(),
llvm::inconvertibleErrorCode());
}
+Replacements Replacements::getCanonicalReplacements() const {
+ std::vector NewReplaces;
+ // Merge adjacent replacements.
+ for (const auto : Replaces) {
+if (NewReplaces.empty()) {
+ NewReplaces.push_back(R);
+ continue;
+}
+auto = NewReplaces.back();
+unsigned PrevEnd = Prev.getOffset() + Prev.getLength();
+if (PrevEnd < R.getOffset()) {
+ NewReplaces.push_back(R);
+} else {
+ assert(PrevEnd == R.getOffset() &&
+ "Existing replacements must not overlap.");
+ Replacement NewR(
+
Re: [PATCH] D24800: Merge conflicting replacements when they are order-independent.
klimek accepted this revision.
klimek added a comment.
This revision is now accepted and ready to land.
lg
Comment at: lib/Tooling/Core/Replacement.cpp:179-181
@@ +178,5 @@
+// `R` and `Replaces` are order-independent if applying them in either order
+// has the same effect, so we need to compare replacements associated to
+// applying them in either order.
+llvm::Expected
+Replacements::mergeIfOrderIndependent(const Replacement ) const {
Much better. Thanks.
https://reviews.llvm.org/D24800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24800: Merge conflicting replacements when they are order-independent.
ioeric updated this revision to Diff 72666.
ioeric marked 3 inline comments as done.
ioeric added a comment.
- Addressed review comments.
https://reviews.llvm.org/D24800
Files:
include/clang/Tooling/Core/Replacement.h
lib/Tooling/Core/Replacement.cpp
unittests/Tooling/RefactoringTest.cpp
Index: unittests/Tooling/RefactoringTest.cpp
===
--- unittests/Tooling/RefactoringTest.cpp
+++ 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,
Re: [PATCH] D24800: Merge conflicting replacements when they are order-independent.
ioeric added inline comments.
Comment at: lib/Tooling/Core/Replacement.cpp:179-181
@@ +178,5 @@
+llvm::Expected
+Replacements::mergeIfOrderIndependent(const Replacement ) const {
+ Replacements Rs(R);
+ Replacements RsShiftedByReplaces(getReplacementInChangedCode(R));
+ Replacements ReplacesShiftedByRs;
klimek wrote:
> I think this is a bit subtle and needs a lot more comments to guide me along
> what is happening and why ...
Ahh, right! Should've done that...
Comments added.
https://reviews.llvm.org/D24800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24800: Merge conflicting replacements when they are order-independent.
klimek added inline comments.
Comment at: lib/Tooling/Core/Replacement.cpp:179-181
@@ +178,5 @@
+llvm::Expected
+Replacements::mergeIfOrderIndependent(const Replacement ) const {
+ Replacements Rs(R);
+ Replacements RsShiftedByReplaces(getReplacementInChangedCode(R));
+ Replacements ReplacesShiftedByRs;
I think this is a bit subtle and needs a lot more comments to guide me along
what is happening and why ...
https://reviews.llvm.org/D24800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24800: Merge conflicting replacements when they are order-independent.
ioeric added inline comments.
Comment at: lib/Tooling/Core/Replacement.cpp:179-181
@@ +178,5 @@
+Replacements::mergeIfOrderIndependent(const Replacement ) const {
+ Replacements Rs(R);
+ Replacements ShiftedRs(getReplacementInChangedCode(R));
+ Replacements ShiftedReplaces;
+ for (const auto : Replaces)
klimek wrote:
> These names are confusing me...
>
Trying to make names less confusing... any better now?
Comment at: lib/Tooling/Core/Replacement.cpp:184
@@ +183,3 @@
+ShiftedReplaces.Replaces.insert(Rs.getReplacementInChangedCode(Replace));
+ auto MergeRs = merge(ShiftedRs);
+ auto MergeReplaces = Rs.merge(ShiftedReplaces);
klimek wrote:
> This comes from a single replacement - why do we need to call merge?
Because it refers to code after `Replaces` are applied?
https://reviews.llvm.org/D24800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24800: Merge conflicting replacements when they are order-independent.
ioeric updated this revision to Diff 72642.
ioeric marked 6 inline comments as done.
ioeric added a comment.
- Addressed review comments.
https://reviews.llvm.org/D24800
Files:
include/clang/Tooling/Core/Replacement.h
lib/Tooling/Core/Replacement.cpp
unittests/Tooling/RefactoringTest.cpp
Index: unittests/Tooling/RefactoringTest.cpp
===
--- unittests/Tooling/RefactoringTest.cpp
+++ 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,
Re: [PATCH] D24800: Merge conflicting replacements when they are order-independent.
klimek added inline comments.
Comment at: include/clang/Tooling/Core/Replacement.h:177-178
@@ +176,4 @@
+ /// - 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.
+ /// Examples:
ioeric wrote:
> klimek wrote:
> > This seems incorrect. What am I missing?
> Here is the discussion about this: https://reviews.llvm.org/D24717
I misunderstood :) Perhaps change to:
, i.e. if X + Y == Y + X when inserting X and Y respectively.
Comment at: include/clang/Tooling/Core/Replacement.h:242
@@ +241,3 @@
+ // If `R` and all existing replacements are order-indepedent, then merge it
+ // with `Replaces` and returns the merged replacements; otheriwse, returns an
+ // error.
typo: otheriwse
Comment at: lib/Tooling/Core/Replacement.cpp:162
@@ +161,3 @@
+auto = NewReplaces.back();
+auto PrevEnd = Prev.getOffset() + Prev.getLength();
+if (PrevEnd < R.getOffset()) {
I'd not use auto for simple integer types.
Comment at: lib/Tooling/Core/Replacement.cpp:166
@@ +165,3 @@
+} else {
+ assert(PrevEnd == R.getOffset());
+ Replacement NewR(
Add comment why we can assert this.
Comment at: lib/Tooling/Core/Replacement.cpp:179-181
@@ +178,5 @@
+Replacements::mergeIfOrderIndependent(const Replacement ) const {
+ Replacements Rs(R);
+ Replacements ShiftedRs(getReplacementInChangedCode(R));
+ Replacements ShiftedReplaces;
+ for (const auto : Replaces)
These names are confusing me...
Comment at: lib/Tooling/Core/Replacement.cpp:184
@@ +183,3 @@
+ShiftedReplaces.Replaces.insert(Rs.getReplacementInChangedCode(Replace));
+ auto MergeRs = merge(ShiftedRs);
+ auto MergeReplaces = Rs.merge(ShiftedReplaces);
This comes from a single replacement - why do we need to call merge?
https://reviews.llvm.org/D24800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24800: Merge conflicting replacements when they are order-independent.
ioeric added inline comments. Comment at: include/clang/Tooling/Core/Replacement.h:177-178 @@ +176,4 @@ + /// - 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. + /// Examples: klimek wrote: > This seems incorrect. What am I missing? Here is the discussion about this: https://reviews.llvm.org/D24717 https://reviews.llvm.org/D24800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24800: Merge conflicting replacements when they are order-independent.
klimek added inline comments. Comment at: include/clang/Tooling/Core/Replacement.h:177-178 @@ +176,4 @@ + /// - 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. + /// Examples: This seems incorrect. What am I missing? https://reviews.llvm.org/D24800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24800: Merge conflicting replacements when they are order-independent.
ioeric added a comment. Friendly ping :) https://reviews.llvm.org/D24800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24800: Merge conflicting replacements when they are order-independent.
ioeric updated this revision to Diff 72045.
ioeric added a comment.
- Provide order-independent replacements examples in comments.
https://reviews.llvm.org/D24800
Files:
include/clang/Tooling/Core/Replacement.h
lib/Tooling/Core/Replacement.cpp
unittests/Tooling/RefactoringTest.cpp
Index: unittests/Tooling/RefactoringTest.cpp
===
--- unittests/Tooling/RefactoringTest.cpp
+++ 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, ""));
+
