maxjhandsome
/
llvm-project
forked from OSchip/llvm-project
1
0
Fork 0
Code Issues Pull Requests Packages Releases Wiki Activity

Supports adding insertion around non-insertion replacements. 

Summary:
Extend `tooling::Replacements::add()` to support adding order-independent replacements.

Two replacements are considered order-independent if one of the following conditions is true:
  - They do not overlap. (This is already supported.)
  - One replacement is insertion, and the other is a replacement with
    length > 0, and the insertion is adjecent to but not contained in the
    other replacement. In this case, the replacement should always change
    the original code instead of the inserted text.

Reviewers: klimek, djasper

Subscribers: cfe-commits, klimek

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

llvm-svn: 281457
This commit is contained in:
Eric Liu 2016-09-14 13:04:51 +00:00
parent c2ed91fc4e
commit ac73ea34a4
3 changed files with 116 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
clang/include/clang/Tooling/Core/Replacement.h
View File

@ -158,14 +158,18 @@ class 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.
/// 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
/// existing replacements or R's file path is different from the filepath of
/// existing replacements. Callers must explicitly check the Error returned.
/// This prevents users from adding 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.
/// existing replacements (i.e. they are order-dependent) or R's file path is
/// different from the filepath of existing replacements. Callers must
/// explicitly check the Error returned. This prevents users from adding
/// 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:
/// - 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
/// for such replacements since users may add them intentionally as a special
/// category of replacements.

44
clang/lib/Tooling/Core/Replacement.cpp
View File

@ -137,6 +137,14 @@ void Replacement::setFromSourceRange(const SourceManager &Sources,
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) {
// Check the file path.
if (!Replaces.empty() && R.getFilePath() != Replaces.begin()->getFilePath())
@ -163,11 +171,22 @@ llvm::Error Replacements::add(const Replacement &R) {
// entries that start at the end can still be conflicting if R is an
// insertion.
auto I = Replaces.lower_bound(AtEnd);
// If it starts at the same offset as R (can only happen if R is an
// insertion), we have a conflict. In that case, increase I to fall through
// to the conflict check.
if (I != Replaces.end() && R.getOffset() == I->getOffset())
++I;
// If `I` starts at the same offset as `R`, `R` must be an insertion.
if (I != Replaces.end() && R.getOffset() == I->getOffset()) {
assert(R.getLength() == 0);
// `I` is also an insertion, `R` and `I` conflict.
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.
// If that is begin(), there are no overlaps.
@ -178,16 +197,19 @@ llvm::Error Replacements::add(const Replacement &R) {
--I;
// If the previous entry does not overlap, we know that entries before it
// can also not overlap.
if (R.getOffset() != I->getOffset() &&
!Range(R.getOffset(), R.getLength())
if (!Range(R.getOffset(), R.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 it has length 0.
// - 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);
return llvm::Error::success();
}
return llvm::make_error<llvm::StringError>(
"New replacement:\n" + R.toString() +
"\nconflicts with existing replacement:\n" + I->toString(),
llvm::inconvertibleErrorCode());
return makeConflictReplacementsError(R, *I);
}
namespace {

77
clang/unittests/Tooling/RefactoringTest.cpp
View File

@ -115,15 +115,16 @@ TEST_F(ReplacementTest, FailAddReplacements) {
llvm::consumeError(std::move(Err));
}
TEST_F(ReplacementTest, FailAddOverlappingInsertions) {
TEST_F(ReplacementTest, AddAdjacentInsertionAndReplacement) {
Replacements Replaces;
// Test adding an insertion at the offset of an existing replacement.
auto Err = Replaces.add(Replacement("x.cc", 10, 3, "replace"));
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement("x.cc", 10, 0, "insert"));
EXPECT_TRUE((bool)Err);
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
EXPECT_EQ(Replaces.size(), 2u);
Replaces.clear();
// Test overlap with an existing insertion.
@ -131,8 +132,9 @@ TEST_F(ReplacementTest, FailAddOverlappingInsertions) {
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement("x.cc", 10, 3, "replace"));
EXPECT_TRUE((bool)Err);
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
EXPECT_EQ(Replaces.size(), 2u);
}
TEST_F(ReplacementTest, FailAddRegression) {
@ -157,14 +159,24 @@ TEST_F(ReplacementTest, FailAddRegression) {
llvm::consumeError(std::move(Err));
}
TEST_F(ReplacementTest, FailAddInsertAtOffsetOfReplacement) {
TEST_F(ReplacementTest, InsertAtOffsetOfReplacement) {
Replacements Replaces;
auto Err = Replaces.add(Replacement("x.cc", 10, 2, ""));
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(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) {
@ -175,6 +187,38 @@ TEST_F(ReplacementTest, FailAddInsertAtOtherInsert) {
Err = Replaces.add(Replacement("x.cc", 10, 0, "b"));
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, 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) {
@ -189,6 +233,29 @@ TEST_F(ReplacementTest, CanApplyReplacements) {
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) {
FileID ID = Context.createInMemoryFile("input.cpp",
"line1\nline2\nline3\nline4");