Re: [PATCH] D21547: Added calculateRangesAfterReplaments() to calculate affacted ranges in the new code.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL273290: Added calculateRangesAfterReplaments() to calculate
affacted ranges in the… (authored by ioeric).
Changed prior to commit:
http://reviews.llvm.org/D21547?vs=61405=61407#toc
Repository:
rL LLVM
http://reviews.llvm.org/D21547
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/lib/Tooling/Core/Replacement.cpp
===
--- cfe/trunk/lib/Tooling/Core/Replacement.cpp
+++ cfe/trunk/lib/Tooling/Core/Replacement.cpp
@@ -278,6 +278,30 @@
return Result;
}
+// Merge and sort overlapping ranges in \p Ranges.
+static std::vector mergeAndSortRanges(std::vector Ranges) {
+ std::sort(Ranges.begin(), Ranges.end(),
+[](const Range , const Range ) {
+ if (LHS.getOffset() != RHS.getOffset())
+return LHS.getOffset() < RHS.getOffset();
+ return LHS.getLength() < RHS.getLength();
+});
+ std::vector Result;
+ for (const auto : Ranges) {
+if (Result.empty() ||
+Result.back().getOffset() + Result.back().getLength() < R.getOffset()) {
+ Result.push_back(R);
+} else {
+ unsigned NewEnd =
+ std::max(Result.back().getOffset() + Result.back().getLength(),
+ R.getOffset() + R.getLength());
+ Result[Result.size() - 1] =
+ Range(Result.back().getOffset(), NewEnd - Result.back().getOffset());
+}
+ }
+ return Result;
+}
+
std::vector calculateChangedRanges(const Replacements ) {
std::vector ChangedRanges;
int Shift = 0;
@@ -287,7 +311,20 @@
Shift += Length - R.getLength();
ChangedRanges.push_back(Range(Offset, Length));
}
- return ChangedRanges;
+ return mergeAndSortRanges(ChangedRanges);
+}
+
+std::vector
+calculateRangesAfterReplacements(const Replacements ,
+ const std::vector ) {
+ auto MergedRanges = mergeAndSortRanges(Ranges);
+ tooling::Replacements FakeReplaces;
+ for (const auto : MergedRanges)
+FakeReplaces.insert(Replacement(Replaces.begin()->getFilePath(),
+R.getOffset(), R.getLength(),
+std::string(" ", R.getLength(;
+ tooling::Replacements NewReplaces = mergeReplacements(FakeReplaces, Replaces);
+ return calculateChangedRanges(NewReplaces);
}
namespace {
@@ -434,4 +471,3 @@
} // end namespace tooling
} // end namespace clang
-
Index: cfe/trunk/unittests/Tooling/RefactoringTest.cpp
===
--- cfe/trunk/unittests/Tooling/RefactoringTest.cpp
+++ cfe/trunk/unittests/Tooling/RefactoringTest.cpp
@@ -462,13 +462,96 @@
std::vector Ranges = calculateChangedRanges(Replaces);
- EXPECT_EQ(3ul, Ranges.size());
+ EXPECT_EQ(2ul, Ranges.size());
EXPECT_TRUE(Ranges[0].getOffset() == 0);
EXPECT_TRUE(Ranges[0].getLength() == 0);
EXPECT_TRUE(Ranges[1].getOffset() == 6);
- EXPECT_TRUE(Ranges[1].getLength() == 6);
- EXPECT_TRUE(Ranges[2].getOffset() == 12);
- EXPECT_TRUE(Ranges[2].getLength() == 16);
+ EXPECT_TRUE(Ranges[1].getLength() == 22);
+}
+
+TEST(Range, RangesAfterReplacements) {
+ std::vector Ranges = {Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 0, 2, "1234")};
+ std::vector Expected = {Range(0, 4), Range(7, 2), Range(12, 5)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesBeforeReplacements) {
+ std::vector Ranges = {Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 20, 2, "1234")};
+ std::vector Expected = {Range(5, 2), Range(10, 5), Range(20, 4)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, NotAffectedByReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 3, 2, "12"),
+ Replacement("foo", 12, 2, "12"),
+ Replacement("foo", 20, 5, "")};
+ std::vector Expected = {Range(0, 2), Range(3, 4), Range(10, 5),
+ Range(20, 0)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesWithNonOverlappingReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 3, 1, ""),
+ Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 20, 2, "12345")};
+ std::vector Expected = {Range(0, 2), Range(3, 0), Range(4, 4),
+ Range(11, 5), Range(21, 5)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesWithOverlappingReplacements) {
+
Re: [PATCH] D21547: Added calculateRangesAfterReplaments() to calculate affacted ranges in the new code.
ioeric added inline comments.
Comment at: lib/Tooling/Core/Replacement.cpp:285
@@ +284,3 @@
+[](const Range , const Range ) -> bool {
+ if (LHS.getOffset() != RHS.getOffset())
+return LHS.getOffset() < RHS.getOffset();
djasper wrote:
> or std::tie(LHS.getOffset(), LHS.getLength()) < std::tie(RHS.getOffset(),
> RHS.getLength())?
I tried std::tie(args...), but it takes references of args...and return values
are temp values.
Comment at: lib/Tooling/Core/Replacement.cpp:292
@@ +291,3 @@
+if (Result.empty() ||
+Result.back().getOffset() + Result.back().getLength() < R.getOffset())
{
+ Result.push_back(R);
djasper wrote:
> Maybe pull out:
> unsigned CurrentEnd = Result.back().getOffset() + Result.back().getLength();
>
> Will probably save a line or two :)
But there is Result.empty()...?
http://reviews.llvm.org/D21547
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21547: Added calculateRangesAfterReplaments() to calculate affacted ranges in the new code.
ioeric updated this revision to Diff 61405.
ioeric marked 2 inline comments as done.
ioeric added a comment.
- removed redundant return type in lambda function.
http://reviews.llvm.org/D21547
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
@@ -462,13 +462,96 @@
std::vector Ranges = calculateChangedRanges(Replaces);
- EXPECT_EQ(3ul, Ranges.size());
+ EXPECT_EQ(2ul, Ranges.size());
EXPECT_TRUE(Ranges[0].getOffset() == 0);
EXPECT_TRUE(Ranges[0].getLength() == 0);
EXPECT_TRUE(Ranges[1].getOffset() == 6);
- EXPECT_TRUE(Ranges[1].getLength() == 6);
- EXPECT_TRUE(Ranges[2].getOffset() == 12);
- EXPECT_TRUE(Ranges[2].getLength() == 16);
+ EXPECT_TRUE(Ranges[1].getLength() == 22);
+}
+
+TEST(Range, RangesAfterReplacements) {
+ std::vector Ranges = {Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 0, 2, "1234")};
+ std::vector Expected = {Range(0, 4), Range(7, 2), Range(12, 5)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesBeforeReplacements) {
+ std::vector Ranges = {Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 20, 2, "1234")};
+ std::vector Expected = {Range(5, 2), Range(10, 5), Range(20, 4)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, NotAffectedByReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 3, 2, "12"),
+ Replacement("foo", 12, 2, "12"),
+ Replacement("foo", 20, 5, "")};
+ std::vector Expected = {Range(0, 2), Range(3, 4), Range(10, 5),
+ Range(20, 0)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesWithNonOverlappingReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 3, 1, ""),
+ Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 20, 2, "12345")};
+ std::vector Expected = {Range(0, 2), Range(3, 0), Range(4, 4),
+ Range(11, 5), Range(21, 5)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesWithOverlappingReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(15, 5),
+ Range(30, 5)};
+ Replacements Replaces = {
+ Replacement("foo", 1, 3, ""), Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 13, 3, "1"), Replacement("foo", 25, 15, "")};
+ std::vector Expected = {Range(0, 1), Range(2, 4), Range(12, 5),
+ Range(22, 0)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, MergeIntoOneRange) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(15, 5)};
+ Replacements Replaces = {Replacement("foo", 1, 15, "1234567890")};
+ std::vector Expected = {Range(0, 15)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, ReplacementsStartingAtRangeOffsets) {
+ std::vector Ranges = {Range(0, 2), Range(5, 5), Range(15, 5)};
+ Replacements Replaces = {
+ Replacement("foo", 0, 2, "12"), Replacement("foo", 5, 1, "123"),
+ Replacement("foo", 7, 4, "12345"), Replacement("foo", 15, 10, "12")};
+ std::vector Expected = {Range(0, 2), Range(5, 9), Range(18, 2)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, ReplacementsEndingAtRangeEnds) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(15, 5)};
+ Replacements Replaces = {Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 17, 3, "12")};
+ std::vector Expected = {Range(0, 2), Range(5, 4), Range(17, 4)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, AjacentReplacements) {
+ std::vector Ranges = {Range(0, 0), Range(15, 5)};
+ Replacements Replaces = {Replacement("foo", 1, 2, "123"),
+ Replacement("foo", 12, 3, "1234")};
+ std::vector Expected = {Range(0, 0), Range(1, 3), Range(13, 9)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, MergeRangesAfterReplacements) {
+ std::vector Ranges = {Range(8, 0), Range(5, 2), Range(9, 0), Range(0, 1)};
+ Replacements Replaces = {Replacement("foo", 1, 3, ""),
+ Replacement("foo", 7, 0, "12"), Replacement("foo", 9, 2, "")};
+ std::vector Expected = {Range(0, 1), Range(2, 4), Range(7, 0), Range(8, 0)};
+ EXPECT_EQ(Expected,
Re: [PATCH] D21547: Added calculateRangesAfterReplaments() to calculate affacted ranges in the new code.
djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.
A couple of small comments, otherwise looks good.
Comment at: lib/Tooling/Core/Replacement.cpp:284
@@ +283,3 @@
+ std::sort(Ranges.begin(), Ranges.end(),
+[](const Range , const Range ) -> bool {
+ if (LHS.getOffset() != RHS.getOffset())
I don't think you need "-> bool".
Comment at: lib/Tooling/Core/Replacement.cpp:285
@@ +284,3 @@
+[](const Range , const Range ) -> bool {
+ if (LHS.getOffset() != RHS.getOffset())
+return LHS.getOffset() < RHS.getOffset();
or std::tie(LHS.getOffset(), LHS.getLength()) < std::tie(RHS.getOffset(),
RHS.getLength())?
Comment at: lib/Tooling/Core/Replacement.cpp:292
@@ +291,3 @@
+if (Result.empty() ||
+Result.back().getOffset() + Result.back().getLength() < R.getOffset())
{
+ Result.push_back(R);
Maybe pull out:
unsigned CurrentEnd = Result.back().getOffset() + Result.back().getLength();
Will probably save a line or two :)
http://reviews.llvm.org/D21547
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21547: Added calculateRangesAfterReplaments() to calculate affacted ranges in the new code.
ioeric added inline comments.
Comment at: include/clang/Tooling/Core/Replacement.h:69
@@ +68,3 @@
+ /// \brief Whether this range equals to \p RHS or not.
+ bool operator==(const Range ) const {
+return Offset == RHS.getOffset() && Length == RHS.getLength();
djasper wrote:
> Why is this required?
It is used in unit test to test the equality between two ranges.
Comment at: lib/Tooling/Core/Replacement.cpp:287
@@ +286,3 @@
+ std::sort(Ranges.begin(), Ranges.end());
+ auto I = Ranges.begin();
+ unsigned CurBegin = I->getOffset();
djasper wrote:
> maybe:
>
> std::vector Result;
> for (const auto : Ranges) {
> if (Result.empty() || !Result.back().overlapsWith(R)) {
> Result.push_back(R);
> } else {
> unsigned NewEnd = std::max(Result.back.getOffset() +
> Result.back.getLength(), R.getOffset() + R.getLength());
> Result[Result.size() - 1] = Range(Result.back.getOffset(), NewEnd -
> Result.back.getOffset());
> }
> }
Thanks! Just one minor point: we also want to merge ranges `R1` and `R2` if
```
R1.getOffset() + R1.getLength() == R2.getOffset()
```
For example, if we have `R1(1, 1)` and `R2(2, 1)`, then we can merge them into
`R3(1, 2)`.
http://reviews.llvm.org/D21547
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21547: Added calculateRangesAfterReplaments() to calculate affacted ranges in the new code.
ioeric updated this revision to Diff 61375.
ioeric marked 5 inline comments as done and an inline comment as not done.
ioeric added a comment.
- Addressed reviewer's comments.
http://reviews.llvm.org/D21547
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
@@ -462,13 +462,96 @@
std::vector Ranges = calculateChangedRanges(Replaces);
- EXPECT_EQ(3ul, Ranges.size());
+ EXPECT_EQ(2ul, Ranges.size());
EXPECT_TRUE(Ranges[0].getOffset() == 0);
EXPECT_TRUE(Ranges[0].getLength() == 0);
EXPECT_TRUE(Ranges[1].getOffset() == 6);
- EXPECT_TRUE(Ranges[1].getLength() == 6);
- EXPECT_TRUE(Ranges[2].getOffset() == 12);
- EXPECT_TRUE(Ranges[2].getLength() == 16);
+ EXPECT_TRUE(Ranges[1].getLength() == 22);
+}
+
+TEST(Range, RangesAfterReplacements) {
+ std::vector Ranges = {Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 0, 2, "1234")};
+ std::vector Expected = {Range(0, 4), Range(7, 2), Range(12, 5)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesBeforeReplacements) {
+ std::vector Ranges = {Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 20, 2, "1234")};
+ std::vector Expected = {Range(5, 2), Range(10, 5), Range(20, 4)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, NotAffectedByReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 3, 2, "12"),
+ Replacement("foo", 12, 2, "12"),
+ Replacement("foo", 20, 5, "")};
+ std::vector Expected = {Range(0, 2), Range(3, 4), Range(10, 5),
+ Range(20, 0)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesWithNonOverlappingReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 3, 1, ""),
+ Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 20, 2, "12345")};
+ std::vector Expected = {Range(0, 2), Range(3, 0), Range(4, 4),
+ Range(11, 5), Range(21, 5)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesWithOverlappingReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(15, 5),
+ Range(30, 5)};
+ Replacements Replaces = {
+ Replacement("foo", 1, 3, ""), Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 13, 3, "1"), Replacement("foo", 25, 15, "")};
+ std::vector Expected = {Range(0, 1), Range(2, 4), Range(12, 5),
+ Range(22, 0)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, MergeIntoOneRange) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(15, 5)};
+ Replacements Replaces = {Replacement("foo", 1, 15, "1234567890")};
+ std::vector Expected = {Range(0, 15)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, ReplacementsStartingAtRangeOffsets) {
+ std::vector Ranges = {Range(0, 2), Range(5, 5), Range(15, 5)};
+ Replacements Replaces = {
+ Replacement("foo", 0, 2, "12"), Replacement("foo", 5, 1, "123"),
+ Replacement("foo", 7, 4, "12345"), Replacement("foo", 15, 10, "12")};
+ std::vector Expected = {Range(0, 2), Range(5, 9), Range(18, 2)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, ReplacementsEndingAtRangeEnds) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(15, 5)};
+ Replacements Replaces = {Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 17, 3, "12")};
+ std::vector Expected = {Range(0, 2), Range(5, 4), Range(17, 4)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, AjacentReplacements) {
+ std::vector Ranges = {Range(0, 0), Range(15, 5)};
+ Replacements Replaces = {Replacement("foo", 1, 2, "123"),
+ Replacement("foo", 12, 3, "1234")};
+ std::vector Expected = {Range(0, 0), Range(1, 3), Range(13, 9)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, MergeRangesAfterReplacements) {
+ std::vector Ranges = {Range(8, 0), Range(5, 2), Range(9, 0), Range(0, 1)};
+ Replacements Replaces = {Replacement("foo", 1, 3, ""),
+ Replacement("foo", 7, 0, "12"), Replacement("foo", 9, 2, "")};
+ std::vector Expected = {Range(0, 1), Range(2, 4), Range(7, 0), Range(8, 0)};
+
Re: [PATCH] D21547: Added calculateRangesAfterReplaments() to calculate affacted ranges in the new code.
djasper added a comment.
Nice :)
Comment at: include/clang/Tooling/Core/Replacement.h:62
@@ +61,3 @@
+ /// \brief Whether this range is less-than \p RHS or not.
+ bool operator<(const Range ) const {
+if (Offset != RHS.getOffset())
Can we just add a lambda that does this to the std::sort operation? I am not
sure that this is actually a natural order for all Replacements.
Comment at: include/clang/Tooling/Core/Replacement.h:69
@@ +68,3 @@
+ /// \brief Whether this range equals to \p RHS or not.
+ bool operator==(const Range ) const {
+return Offset == RHS.getOffset() && Length == RHS.getLength();
Why is this required?
Comment at: lib/Tooling/Core/Replacement.cpp:287
@@ +286,3 @@
+ std::sort(Ranges.begin(), Ranges.end());
+ auto I = Ranges.begin();
+ unsigned CurBegin = I->getOffset();
maybe:
std::vector Result;
for (const auto : Ranges) {
if (Result.empty() || !Result.back().overlapsWith(R)) {
Result.push_back(R);
} else {
unsigned NewEnd = std::max(Result.back.getOffset() +
Result.back.getLength(), R.getOffset() + R.getLength());
Result[Result.size() - 1] = Range(Result.back.getOffset(), NewEnd -
Result.back.getOffset());
}
}
Comment at: lib/Tooling/Core/Replacement.cpp:319
@@ +318,3 @@
+ const std::vector ) {
+ if (Ranges.empty())
+return calculateChangedRanges(Replaces);
I'd probably leave these early exits out. I don't think anyone will ever notice
the performance difference.
Comment at: lib/Tooling/Core/Replacement.cpp:321
@@ +320,3 @@
+return calculateChangedRanges(Replaces);
+
+ if (Replaces.empty())
I think you need to do:
Ranges = mergeAndSortRanges(Ranges);
here
http://reviews.llvm.org/D21547
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21547: Added calculateRangesAfterReplaments() to calculate affacted ranges in the new code.
ioeric added inline comments.
Comment at: include/clang/Tooling/Core/Replacement.h:253
@@ +252,3 @@
+std::vector
+calculateRangesAfterReplacements(const Replacements ,
+ const std::vector );
getShiftedRanges sounds like it's excluding affected ranges of Replaces. I
think often time we want both?
Comment at: include/clang/Tooling/Core/Replacement.h:237
@@ +236,3 @@
+/// \brief Calculates the new ranges after \p Replaces are applied. These
+/// include both the original \p Ranges and the affected ranges of \p Replaces
+/// in the new code.
Why would why want to exclude the newly affected ranges? But if replaces
overlap with original ranges, do we consider sub-ranges in original ranges
being replaced as parts of the shifted ranges in the new code?
Comment at: lib/Tooling/Core/Replacement.cpp:305
@@ +304,3 @@
+// Merge and sort overlapping and adjacent ranges in \p Ranges.
+static void mergeAndSortRanges(std::vector ) {
+ if (Ranges.empty())
Shall we pass Ranges by value instead since it's gonna be sorted anyway?
Also, does it make sense to have calculateChangedRanges() call this?
Comment at: lib/Tooling/Core/Replacement.cpp:328
@@ +327,3 @@
+std::vector
+calculateRangesAfterReplacements(const Replacements ,
+ std::vector Ranges) {
That actually works very well! Thanks!
http://reviews.llvm.org/D21547
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21547: Added calculateRangesAfterReplaments() to calculate affacted ranges in the new code.
ioeric updated this revision to Diff 61359.
ioeric marked 6 inline comments as done.
ioeric added a comment.
- Use mergeReplacements to calculate the new affected ranges.
http://reviews.llvm.org/D21547
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
@@ -462,13 +462,96 @@
std::vector Ranges = calculateChangedRanges(Replaces);
- EXPECT_EQ(3ul, Ranges.size());
+ EXPECT_EQ(2ul, Ranges.size());
EXPECT_TRUE(Ranges[0].getOffset() == 0);
EXPECT_TRUE(Ranges[0].getLength() == 0);
EXPECT_TRUE(Ranges[1].getOffset() == 6);
- EXPECT_TRUE(Ranges[1].getLength() == 6);
- EXPECT_TRUE(Ranges[2].getOffset() == 12);
- EXPECT_TRUE(Ranges[2].getLength() == 16);
+ EXPECT_TRUE(Ranges[1].getLength() == 22);
+}
+
+TEST(Range, RangesAfterReplacements) {
+ std::vector Ranges = {Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 0, 2, "1234")};
+ std::vector Expected = {Range(0, 4), Range(7, 2), Range(12, 5)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesBeforeReplacements) {
+ std::vector Ranges = {Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 20, 2, "1234")};
+ std::vector Expected = {Range(5, 2), Range(10, 5), Range(20, 4)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, NotAffectedByReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 3, 2, "12"),
+ Replacement("foo", 12, 2, "12"),
+ Replacement("foo", 20, 5, "")};
+ std::vector Expected = {Range(0, 2), Range(3, 4), Range(10, 5),
+ Range(20, 0)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesWithNonOverlappingReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 3, 1, ""),
+ Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 20, 2, "12345")};
+ std::vector Expected = {Range(0, 2), Range(3, 0), Range(4, 4),
+ Range(11, 5), Range(21, 5)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesWithOverlappingReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(15, 5),
+ Range(30, 5)};
+ Replacements Replaces = {
+ Replacement("foo", 1, 3, ""), Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 13, 3, "1"), Replacement("foo", 25, 15, "")};
+ std::vector Expected = {Range(0, 1), Range(2, 4), Range(12, 5),
+ Range(22, 0)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, MergeIntoOneRange) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(15, 5)};
+ Replacements Replaces = {Replacement("foo", 1, 15, "1234567890")};
+ std::vector Expected = {Range(0, 15)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, ReplacementsStartingAtRangeOffsets) {
+ std::vector Ranges = {Range(0, 2), Range(5, 5), Range(15, 5)};
+ Replacements Replaces = {
+ Replacement("foo", 0, 2, "12"), Replacement("foo", 5, 1, "123"),
+ Replacement("foo", 7, 4, "12345"), Replacement("foo", 15, 10, "12")};
+ std::vector Expected = {Range(0, 2), Range(5, 9), Range(18, 2)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, ReplacementsEndingAtRangeEnds) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(15, 5)};
+ Replacements Replaces = {Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 17, 3, "12")};
+ std::vector Expected = {Range(0, 2), Range(5, 4), Range(17, 4)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, AjacentReplacements) {
+ std::vector Ranges = {Range(0, 0), Range(15, 5)};
+ Replacements Replaces = {Replacement("foo", 1, 2, "123"),
+ Replacement("foo", 12, 3, "1234")};
+ std::vector Expected = {Range(0, 0), Range(1, 3), Range(13, 9)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, MergeRangesAfterReplacements) {
+ std::vector Ranges = {Range(8, 0), Range(5, 2), Range(9, 0), Range(0, 1)};
+ Replacements Replaces = {Replacement("foo", 1, 3, ""),
+ Replacement("foo", 7, 0, "12"), Replacement("foo", 9, 2, "")};
+ std::vector Expected = {Range(0, 1), Range(2, 4), Range(7, 0), Range(8, 0)};
+ EXPECT_EQ(Expected,
Re: [PATCH] D21547: Added calculateRangesAfterReplaments() to calculate affacted ranges in the new code.
djasper added inline comments.
Comment at: include/clang/Tooling/Core/Replacement.h:68
@@ +67,3 @@
+/// \brief Less-than operator between two Ranges.
+bool operator<(const Range , const Range );
+
Can we keep these private for now?
Comment at: include/clang/Tooling/Core/Replacement.h:237
@@ +236,3 @@
+/// \brief Calculates the new ranges after \p Replaces are applied. These
+/// include both the original \p Ranges and the affected ranges of \p Replaces
+/// in the new code.
Should we make the latter optional?
Comment at: include/clang/Tooling/Core/Replacement.h:240
@@ +239,3 @@
+///
+/// \pre Replacemments must be for the same file.
+///
s/Replacemments/Replacements/
Comment at: include/clang/Tooling/Core/Replacement.h:245
@@ +244,3 @@
+std::vector
+calculateRangesAfterReplacements(const Replacements ,
+ std::vector Ranges);
I'd call this getShiftedRanges().
Comment at: include/clang/Tooling/Core/Replacement.h:246
@@ +245,3 @@
+calculateRangesAfterReplacements(const Replacements ,
+ std::vector Ranges);
+
const vector&?
Comment at: lib/Tooling/Core/Replacement.cpp:39
@@ +38,3 @@
+bool operator==(const Range , const Range ) {
+ return (LHS.getOffset() == RHS.getOffset()) &&
+ (LHS.getLength() == RHS.getLength());
No need for parentheses.
Comment at: lib/Tooling/Core/Replacement.cpp:304
@@ -292,1 +303,3 @@
+// Returns a sorted ranges after merging all overlapping ranges.
+static void mergeAndSortRanges(std::vector ) {
Remove " a"
Comment at: lib/Tooling/Core/Replacement.cpp:305
@@ +304,3 @@
+// Returns a sorted ranges after merging all overlapping ranges.
+static void mergeAndSortRanges(std::vector ) {
+ if (Ranges.empty())
Just return the new vector and use a const parameter.
Comment at: lib/Tooling/Core/Replacement.cpp:328
@@ +327,3 @@
+std::vector
+calculateRangesAfterReplacements(const Replacements ,
+ std::vector Ranges) {
Couldn't we implement this using mergeReplacements?
Fundamentally:
- Turn Ranges into Replacements at (offset, length) with an empty (unimportant)
replacement text of length "length".
- Merge these into Replaces
- Convert the resulting Replacements into Ranges, each with the Replacement's
(offset, replacement_text.size()) and shifting subsequent ones forward.
http://reviews.llvm.org/D21547
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21547: Added calculateRangesAfterReplaments() to calculate affacted ranges in the new code.
ioeric updated this revision to Diff 61340.
ioeric added a comment.
- fixed commenting.
http://reviews.llvm.org/D21547
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
@@ -471,6 +471,91 @@
EXPECT_TRUE(Ranges[2].getLength() == 16);
}
+TEST(Range, RangesAfterReplacements) {
+ std::vector Ranges = {Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 0, 2, "1234")};
+ std::vector Expected = {Range(0, 4), Range(7, 2), Range(12, 5)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesBeforeReplacements) {
+ std::vector Ranges = {Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 20, 2, "1234")};
+ std::vector Expected = {Range(5, 2), Range(10, 5), Range(20, 4)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, NotAffectedByReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 3, 2, "12"),
+ Replacement("foo", 12, 2, "12"),
+ Replacement("foo", 20, 5, "")};
+ std::vector Expected = {Range(0, 2), Range(3, 4), Range(10, 5),
+ Range(20, 0)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesWithNonOverlappingReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 3, 1, ""),
+ Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 20, 2, "12345")};
+ std::vector Expected = {Range(0, 2), Range(3, 0), Range(4, 4),
+ Range(11, 5), Range(21, 5)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesWithOverlappingReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(15, 5),
+ Range(30, 5)};
+ Replacements Replaces = {
+ Replacement("foo", 1, 3, ""), Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 13, 3, "1"), Replacement("foo", 25, 15, "")};
+ std::vector Expected = {Range(0, 1), Range(2, 4), Range(12, 5),
+ Range(22, 0)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, MergeIntoOneRange) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(15, 5)};
+ Replacements Replaces = {Replacement("foo", 1, 15, "1234567890")};
+ std::vector Expected = {Range(0, 15)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, ReplacementsStartingAtRangeOffsets) {
+ std::vector Ranges = {Range(0, 2), Range(5, 5), Range(15, 5)};
+ Replacements Replaces = {
+ Replacement("foo", 0, 2, "12"), Replacement("foo", 5, 1, "123"),
+ Replacement("foo", 7, 4, "12345"), Replacement("foo", 15, 10, "12")};
+ std::vector Expected = {Range(0, 2), Range(5, 9), Range(18, 2)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, ReplacementsEndingAtRangeEnds) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(15, 5)};
+ Replacements Replaces = {Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 17, 3, "12")};
+ std::vector Expected = {Range(0, 2), Range(5, 4), Range(17, 4)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, AjacentReplacements) {
+ std::vector Ranges = {Range(0, 0), Range(15, 5)};
+ Replacements Replaces = {Replacement("foo", 1, 2, "123"),
+ Replacement("foo", 12, 3, "1234")};
+ std::vector Expected = {Range(0, 0), Range(1, 3), Range(13, 9)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, MergeRangesAfterReplacements) {
+ std::vector Ranges = {Range(8, 0), Range(5, 2), Range(9, 0), Range(0, 1)};
+ Replacements Replaces = {Replacement("foo", 1, 3, ""),
+ Replacement("foo", 7, 0, "12"), Replacement("foo", 9, 2, "")};
+ std::vector Expected = {Range(0, 1), Range(2, 4), Range(7, 0), Range(8, 0)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
TEST(DeduplicateTest, removesDuplicates) {
std::vector Input;
Input.push_back(Replacement("fileA", 50, 0, " foo "));
Index: lib/Tooling/Core/Replacement.cpp
===
--- lib/Tooling/Core/Replacement.cpp
+++ lib/Tooling/Core/Replacement.cpp
@@ -29,6 +29,17 @@
static const char * const InvalidLocation = "";
+bool operator<(const Range , const Range )
[PATCH] D21547: Added calculateRangesAfterReplaments() to calculate affacted ranges in the new code.
ioeric created this revision.
ioeric added reviewers: djasper, klimek.
ioeric added a subscriber: cfe-commits.
Herald added a subscriber: klimek.
Added calculateRangesAfterReplaments() to calculate original ranges as well as
newly affacted ranges in the new code.
http://reviews.llvm.org/D21547
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
@@ -471,6 +471,91 @@
EXPECT_TRUE(Ranges[2].getLength() == 16);
}
+TEST(Range, RangesAfterReplacements) {
+ std::vector Ranges = {Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 0, 2, "1234")};
+ std::vector Expected = {Range(0, 4), Range(7, 2), Range(12, 5)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesBeforeReplacements) {
+ std::vector Ranges = {Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 20, 2, "1234")};
+ std::vector Expected = {Range(5, 2), Range(10, 5), Range(20, 4)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, NotAffectedByReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 3, 2, "12"),
+ Replacement("foo", 12, 2, "12"),
+ Replacement("foo", 20, 5, "")};
+ std::vector Expected = {Range(0, 2), Range(3, 4), Range(10, 5),
+ Range(20, 0)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesWithNonOverlappingReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(10, 5)};
+ Replacements Replaces = {Replacement("foo", 3, 1, ""),
+ Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 20, 2, "12345")};
+ std::vector Expected = {Range(0, 2), Range(3, 0), Range(4, 4),
+ Range(11, 5), Range(21, 5)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, RangesWithOverlappingReplacements) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(15, 5),
+ Range(30, 5)};
+ Replacements Replaces = {
+ Replacement("foo", 1, 3, ""), Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 13, 3, "1"), Replacement("foo", 25, 15, "")};
+ std::vector Expected = {Range(0, 1), Range(2, 4), Range(12, 5),
+ Range(22, 0)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, MergeIntoOneRange) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(15, 5)};
+ Replacements Replaces = {Replacement("foo", 1, 15, "1234567890")};
+ std::vector Expected = {Range(0, 15)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, ReplacementsStartingAtRangeOffsets) {
+ std::vector Ranges = {Range(0, 2), Range(5, 5), Range(15, 5)};
+ Replacements Replaces = {
+ Replacement("foo", 0, 2, "12"), Replacement("foo", 5, 1, "123"),
+ Replacement("foo", 7, 4, "12345"), Replacement("foo", 15, 10, "12")};
+ std::vector Expected = {Range(0, 2), Range(5, 9), Range(18, 2)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, ReplacementsEndingAtRangeEnds) {
+ std::vector Ranges = {Range(0, 2), Range(5, 2), Range(15, 5)};
+ Replacements Replaces = {Replacement("foo", 6, 1, "123"),
+ Replacement("foo", 17, 3, "12")};
+ std::vector Expected = {Range(0, 2), Range(5, 4), Range(17, 4)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, AjacentReplacements) {
+ std::vector Ranges = {Range(0, 0), Range(15, 5)};
+ Replacements Replaces = {Replacement("foo", 1, 2, "123"),
+ Replacement("foo", 12, 3, "1234")};
+ std::vector Expected = {Range(0, 0), Range(1, 3), Range(13, 9)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
+TEST(Range, MergeRangesAfterReplacements) {
+ std::vector Ranges = {Range(8, 0), Range(5, 2), Range(9, 0), Range(0, 1)};
+ Replacements Replaces = {Replacement("foo", 1, 3, ""),
+ Replacement("foo", 7, 0, "12"), Replacement("foo", 9, 2, "")};
+ std::vector Expected = {Range(0, 1), Range(2, 4), Range(7, 0), Range(8, 0)};
+ EXPECT_EQ(Expected, calculateRangesAfterReplacements(Replaces, Ranges));
+}
+
TEST(DeduplicateTest, removesDuplicates) {
std::vector Input;
Input.push_back(Replacement("fileA", 50, 0, " foo "));
Index: lib/Tooling/Core/Replacement.cpp
===
---
