Skip to content
Snippets Groups Projects
Commit 40a68ac1 authored by Eric Liu's avatar Eric Liu
Browse files

Recommit r281457 "Supports adding insertion around non-insertion replacements".

Summary:
Diff to r281457:
- added a test case `CalculateRangesOfInsertionAroundReplacement`.

Reviewers: djasper

Subscribers: cfe-commits, klimek

Differential Revision: https://reviews.llvm.org/D24606

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@281891 91177308-0d34-0410-b5e6-96231b3b80d8
parent 812eb3cb
No related branches found
No related tags found
No related merge requests found
...@@ -159,14 +159,18 @@ class Replacements { ...@@ -159,14 +159,18 @@ class Replacements {
/// \brief Adds a new replacement \p R to the current set of replacements. /// \brief Adds a new replacement \p R to the current set of replacements.
/// \p R must have the same file path as all existing replacements. /// \p R must have the same file path as all existing replacements.
/// Returns true if the replacement is successfully inserted; otherwise, /// Returns `success` if the replacement is successfully inserted; otherwise,
/// it returns an llvm::Error, i.e. there is a conflict between R and the /// it returns an llvm::Error, i.e. there is a conflict between R and the
/// existing replacements or R's file path is different from the filepath of /// existing replacements (i.e. they are order-dependent) or R's file path is
/// existing replacements. Callers must explicitly check the Error returned. /// different from the filepath of existing replacements. Callers must
/// This prevents users from adding order-dependent replacements. To control /// explicitly check the Error returned. This prevents users from adding
/// the order in which order-dependent replacements are applied, use /// order-dependent replacements. To control the order in which
/// merge({R}) with R referring to the changed code after applying all /// order-dependent replacements are applied, use merge({R}) with R referring
/// existing replacements. /// to the changed code after applying all existing replacements.
/// Two replacements are considered order-independent if they:
/// - don't overlap (being directly adjacent is fine) and
/// - aren't both inserts at the same code location (would be
/// order-dependent).
/// Replacements with offset UINT_MAX are special - we do not detect conflicts /// Replacements with offset UINT_MAX are special - we do not detect conflicts
/// for such replacements since users may add them intentionally as a special /// for such replacements since users may add them intentionally as a special
/// category of replacements. /// category of replacements.
......
...@@ -134,6 +134,14 @@ void Replacement::setFromSourceRange(const SourceManager &Sources, ...@@ -134,6 +134,14 @@ void Replacement::setFromSourceRange(const SourceManager &Sources,
ReplacementText); ReplacementText);
} }
llvm::Error makeConflictReplacementsError(const Replacement &New,
const Replacement &Existing) {
return llvm::make_error<llvm::StringError>(
"New replacement:\n" + New.toString() +
"\nconflicts with existing replacement:\n" + Existing.toString(),
llvm::inconvertibleErrorCode());
}
llvm::Error Replacements::add(const Replacement &R) { llvm::Error Replacements::add(const Replacement &R) {
// Check the file path. // Check the file path.
if (!Replaces.empty() && R.getFilePath() != Replaces.begin()->getFilePath()) if (!Replaces.empty() && R.getFilePath() != Replaces.begin()->getFilePath())
...@@ -160,13 +168,24 @@ llvm::Error Replacements::add(const Replacement &R) { ...@@ -160,13 +168,24 @@ llvm::Error Replacements::add(const Replacement &R) {
// entries that start at the end can still be conflicting if R is an // entries that start at the end can still be conflicting if R is an
// insertion. // insertion.
auto I = Replaces.lower_bound(AtEnd); auto I = Replaces.lower_bound(AtEnd);
// If it starts at the same offset as R (can only happen if R is an // If `I` starts at the same offset as `R`, `R` must be an insertion.
// insertion), we have a conflict. In that case, increase I to fall through if (I != Replaces.end() && R.getOffset() == I->getOffset()) {
// to the conflict check. assert(R.getLength() == 0);
if (I != Replaces.end() && R.getOffset() == I->getOffset()) // `I` is also an insertion, `R` and `I` conflict.
++I; if (I->getLength() == 0)
return makeConflictReplacementsError(R, *I);
// Insertion `R` is adjacent to a non-insertion replacement `I`, so they
// are order-independent. It is safe to assume that `R` will not conflict
// with any replacement before `I` since all replacements before `I` must
// either end before `R` or end at `R` but has length > 0 (if the
// replacement before `I` is an insertion at `R`, it would have been `I`
// since it is a lower bound of `AtEnd` and ordered before the current `I`
// in the set).
Replaces.insert(R);
return llvm::Error::success();
}
// I is the smallest iterator whose entry cannot overlap. // `I` is the smallest iterator (after `R`) whose entry cannot overlap.
// If that is begin(), there are no overlaps. // If that is begin(), there are no overlaps.
if (I == Replaces.begin()) { if (I == Replaces.begin()) {
Replaces.insert(R); Replaces.insert(R);
...@@ -175,16 +194,19 @@ llvm::Error Replacements::add(const Replacement &R) { ...@@ -175,16 +194,19 @@ llvm::Error Replacements::add(const Replacement &R) {
--I; --I;
// If the previous entry does not overlap, we know that entries before it // If the previous entry does not overlap, we know that entries before it
// can also not overlap. // can also not overlap.
if (R.getOffset() != I->getOffset() && if (!Range(R.getOffset(), R.getLength())
!Range(R.getOffset(), R.getLength())
.overlapsWith(Range(I->getOffset(), I->getLength()))) { .overlapsWith(Range(I->getOffset(), I->getLength()))) {
// If `R` and `I` do not have the same offset, it is safe to add `R` since
// it must come after `I`. Otherwise:
// - If `R` is an insertion, `I` must not be an insertion since it would
// have come after `AtEnd`.
// - If `R` is not an insertion, `I` must be an insertion; otherwise, `R`
// and `I` would have overlapped.
// In either case, we can safely insert `R`.
Replaces.insert(R); Replaces.insert(R);
return llvm::Error::success(); return llvm::Error::success();
} }
return llvm::make_error<llvm::StringError>( return makeConflictReplacementsError(R, *I);
"New replacement:\n" + R.toString() +
"\nconflicts with existing replacement:\n" + I->toString(),
llvm::inconvertibleErrorCode());
} }
namespace { namespace {
......
...@@ -115,15 +115,16 @@ TEST_F(ReplacementTest, FailAddReplacements) { ...@@ -115,15 +115,16 @@ TEST_F(ReplacementTest, FailAddReplacements) {
llvm::consumeError(std::move(Err)); llvm::consumeError(std::move(Err));
} }
TEST_F(ReplacementTest, FailAddOverlappingInsertions) { TEST_F(ReplacementTest, AddAdjacentInsertionAndReplacement) {
Replacements Replaces; Replacements Replaces;
// Test adding an insertion at the offset of an existing replacement. // Test adding an insertion at the offset of an existing replacement.
auto Err = Replaces.add(Replacement("x.cc", 10, 3, "replace")); auto Err = Replaces.add(Replacement("x.cc", 10, 3, "replace"));
EXPECT_TRUE(!Err); EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err)); llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement("x.cc", 10, 0, "insert")); Err = Replaces.add(Replacement("x.cc", 10, 0, "insert"));
EXPECT_TRUE((bool)Err); EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err)); llvm::consumeError(std::move(Err));
EXPECT_EQ(Replaces.size(), 2u);
Replaces.clear(); Replaces.clear();
// Test overlap with an existing insertion. // Test overlap with an existing insertion.
...@@ -131,8 +132,9 @@ TEST_F(ReplacementTest, FailAddOverlappingInsertions) { ...@@ -131,8 +132,9 @@ TEST_F(ReplacementTest, FailAddOverlappingInsertions) {
EXPECT_TRUE(!Err); EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err)); llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement("x.cc", 10, 3, "replace")); Err = Replaces.add(Replacement("x.cc", 10, 3, "replace"));
EXPECT_TRUE((bool)Err); EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err)); llvm::consumeError(std::move(Err));
EXPECT_EQ(Replaces.size(), 2u);
} }
TEST_F(ReplacementTest, FailAddRegression) { TEST_F(ReplacementTest, FailAddRegression) {
...@@ -157,14 +159,24 @@ TEST_F(ReplacementTest, FailAddRegression) { ...@@ -157,14 +159,24 @@ TEST_F(ReplacementTest, FailAddRegression) {
llvm::consumeError(std::move(Err)); llvm::consumeError(std::move(Err));
} }
TEST_F(ReplacementTest, FailAddInsertAtOffsetOfReplacement) { TEST_F(ReplacementTest, InsertAtOffsetOfReplacement) {
Replacements Replaces; Replacements Replaces;
auto Err = Replaces.add(Replacement("x.cc", 10, 2, "")); auto Err = Replaces.add(Replacement("x.cc", 10, 2, ""));
EXPECT_TRUE(!Err); EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err)); llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement("x.cc", 10, 0, "")); Err = Replaces.add(Replacement("x.cc", 10, 0, ""));
EXPECT_TRUE((bool)Err); EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err)); llvm::consumeError(std::move(Err));
EXPECT_EQ(Replaces.size(), 2u);
Replaces.clear();
Err = Replaces.add(Replacement("x.cc", 10, 0, ""));
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement("x.cc", 10, 2, ""));
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
EXPECT_EQ(Replaces.size(), 2u);
} }
TEST_F(ReplacementTest, FailAddInsertAtOtherInsert) { TEST_F(ReplacementTest, FailAddInsertAtOtherInsert) {
...@@ -175,6 +187,38 @@ TEST_F(ReplacementTest, FailAddInsertAtOtherInsert) { ...@@ -175,6 +187,38 @@ TEST_F(ReplacementTest, FailAddInsertAtOtherInsert) {
Err = Replaces.add(Replacement("x.cc", 10, 0, "b")); Err = Replaces.add(Replacement("x.cc", 10, 0, "b"));
EXPECT_TRUE((bool)Err); EXPECT_TRUE((bool)Err);
llvm::consumeError(std::move(Err)); llvm::consumeError(std::move(Err));
Replaces.clear();
Err = Replaces.add(Replacement("x.cc", 10, 0, ""));
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement("x.cc", 10, 0, ""));
EXPECT_TRUE((bool)Err);
llvm::consumeError(std::move(Err));
Replaces.clear();
Err = Replaces.add(Replacement("x.cc", 10, 0, ""));
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement("x.cc", 10, 3, ""));
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement("x.cc", 10, 0, ""));
EXPECT_TRUE((bool)Err);
llvm::consumeError(std::move(Err));
}
TEST_F(ReplacementTest, InsertBetweenAdjacentReplacements) {
Replacements Replaces;
auto Err = Replaces.add(Replacement("x.cc", 10, 5, "a"));
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement("x.cc", 8, 2, "a"));
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement("x.cc", 10, 0, "b"));
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
} }
TEST_F(ReplacementTest, CanApplyReplacements) { TEST_F(ReplacementTest, CanApplyReplacements) {
...@@ -189,6 +233,29 @@ TEST_F(ReplacementTest, CanApplyReplacements) { ...@@ -189,6 +233,29 @@ TEST_F(ReplacementTest, CanApplyReplacements) {
EXPECT_EQ("line1\nreplaced\nother\nline4", Context.getRewrittenText(ID)); EXPECT_EQ("line1\nreplaced\nother\nline4", Context.getRewrittenText(ID));
} }
// Verifies that replacement/deletion is applied before insertion at the same
// offset.
TEST_F(ReplacementTest, InsertAndDelete) {
FileID ID = Context.createInMemoryFile("input.cpp",
"line1\nline2\nline3\nline4");
Replacements Replaces = toReplacements(
{Replacement(Context.Sources, Context.getLocation(ID, 2, 1), 6, ""),
Replacement(Context.Sources, Context.getLocation(ID, 2, 1), 0,
"other\n")});
EXPECT_TRUE(applyAllReplacements(Replaces, Context.Rewrite));
EXPECT_EQ("line1\nother\nline3\nline4", Context.getRewrittenText(ID));
}
TEST_F(ReplacementTest, AdjacentReplacements) {
FileID ID = Context.createInMemoryFile("input.cpp",
"ab");
Replacements Replaces = toReplacements(
{Replacement(Context.Sources, Context.getLocation(ID, 1, 1), 1, "x"),
Replacement(Context.Sources, Context.getLocation(ID, 1, 2), 1, "y")});
EXPECT_TRUE(applyAllReplacements(Replaces, Context.Rewrite));
EXPECT_EQ("xy", Context.getRewrittenText(ID));
}
TEST_F(ReplacementTest, SkipsDuplicateReplacements) { TEST_F(ReplacementTest, SkipsDuplicateReplacements) {
FileID ID = Context.createInMemoryFile("input.cpp", FileID ID = Context.createInMemoryFile("input.cpp",
"line1\nline2\nline3\nline4"); "line1\nline2\nline3\nline4");
...@@ -506,6 +573,17 @@ TEST(Range, CalculateRangesOfReplacements) { ...@@ -506,6 +573,17 @@ TEST(Range, CalculateRangesOfReplacements) {
EXPECT_TRUE(Ranges[1].getLength() == 22); EXPECT_TRUE(Ranges[1].getLength() == 22);
} }
TEST(Range, CalculateRangesOfInsertionAroundReplacement) {
Replacements Replaces = toReplacements(
{Replacement("foo", 0, 2, ""), Replacement("foo", 0, 0, "ba")});
std::vector<Range> Ranges = Replaces.getAffectedRanges();
EXPECT_EQ(1ul, Ranges.size());
EXPECT_EQ(0u, Ranges[0].getOffset());
EXPECT_EQ(2u, Ranges[0].getLength());
}
TEST(Range, RangesAfterEmptyReplacements) { TEST(Range, RangesAfterEmptyReplacements) {
std::vector<Range> Ranges = {Range(5, 6), Range(10, 5)}; std::vector<Range> Ranges = {Range(5, 6), Range(10, 5)};
Replacements Replaces; Replacements Replaces;
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment