r281557 - Revert r281457 "Supports adding insertion around non-insertion replacements."
Artem Belevich via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 14 16:03:06 PDT 2016
Author: tra
Date: Wed Sep 14 18:03:06 2016
New Revision: 281557
URL: http://llvm.org/viewvc/llvm-project?rev=281557&view=rev
Log:
Revert r281457 "Supports adding insertion around non-insertion replacements."
Commit was breaking our internal tests.
Modified:
cfe/trunk/include/clang/Tooling/Core/Replacement.h
cfe/trunk/lib/Tooling/Core/Replacement.cpp
cfe/trunk/unittests/Tooling/RefactoringTest.cpp
Modified: cfe/trunk/include/clang/Tooling/Core/Replacement.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Core/Replacement.h?rev=281557&r1=281556&r2=281557&view=diff
==============================================================================
--- cfe/trunk/include/clang/Tooling/Core/Replacement.h (original)
+++ cfe/trunk/include/clang/Tooling/Core/Replacement.h Wed Sep 14 18:03:06 2016
@@ -158,18 +158,14 @@ 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 `success` if the replacement is successfully inserted; otherwise,
+ /// Returns true if the replacement is successfully inserted; otherwise,
/// it returns an llvm::Error, i.e. there is a conflict between R and the
- /// 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).
+ /// 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.
/// 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.
Modified: cfe/trunk/lib/Tooling/Core/Replacement.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Core/Replacement.cpp?rev=281557&r1=281556&r2=281557&view=diff
==============================================================================
--- cfe/trunk/lib/Tooling/Core/Replacement.cpp (original)
+++ cfe/trunk/lib/Tooling/Core/Replacement.cpp Wed Sep 14 18:03:06 2016
@@ -137,14 +137,6 @@ void Replacement::setFromSourceRange(con
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())
@@ -171,22 +163,11 @@ llvm::Error Replacements::add(const Repl
// entries that start at the end can still be conflicting if R is an
// insertion.
auto I = Replaces.lower_bound(AtEnd);
- // 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();
- }
+ // 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;
// I is the smallest iterator whose entry cannot overlap.
// If that is begin(), there are no overlaps.
@@ -197,19 +178,16 @@ llvm::Error Replacements::add(const Repl
--I;
// If the previous entry does not overlap, we know that entries before it
// can also not overlap.
- if (!Range(R.getOffset(), R.getLength())
+ if (R.getOffset() != I->getOffset() &&
+ !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 makeConflictReplacementsError(R, *I);
+ return llvm::make_error<llvm::StringError>(
+ "New replacement:\n" + R.toString() +
+ "\nconflicts with existing replacement:\n" + I->toString(),
+ llvm::inconvertibleErrorCode());
}
namespace {
Modified: cfe/trunk/unittests/Tooling/RefactoringTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/RefactoringTest.cpp?rev=281557&r1=281556&r2=281557&view=diff
==============================================================================
--- cfe/trunk/unittests/Tooling/RefactoringTest.cpp (original)
+++ cfe/trunk/unittests/Tooling/RefactoringTest.cpp Wed Sep 14 18:03:06 2016
@@ -115,16 +115,15 @@ TEST_F(ReplacementTest, FailAddReplaceme
llvm::consumeError(std::move(Err));
}
-TEST_F(ReplacementTest, AddAdjacentInsertionAndReplacement) {
+TEST_F(ReplacementTest, FailAddOverlappingInsertions) {
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(!Err);
+ EXPECT_TRUE((bool)Err);
llvm::consumeError(std::move(Err));
- EXPECT_EQ(Replaces.size(), 2u);
Replaces.clear();
// Test overlap with an existing insertion.
@@ -132,9 +131,8 @@ TEST_F(ReplacementTest, AddAdjacentInser
EXPECT_TRUE(!Err);
llvm::consumeError(std::move(Err));
Err = Replaces.add(Replacement("x.cc", 10, 3, "replace"));
- EXPECT_TRUE(!Err);
+ EXPECT_TRUE((bool)Err);
llvm::consumeError(std::move(Err));
- EXPECT_EQ(Replaces.size(), 2u);
}
TEST_F(ReplacementTest, FailAddRegression) {
@@ -159,24 +157,14 @@ TEST_F(ReplacementTest, FailAddRegressio
llvm::consumeError(std::move(Err));
}
-TEST_F(ReplacementTest, InsertAtOffsetOfReplacement) {
+TEST_F(ReplacementTest, FailAddInsertAtOffsetOfReplacement) {
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(!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);
+ EXPECT_TRUE((bool)Err);
llvm::consumeError(std::move(Err));
- EXPECT_EQ(Replaces.size(), 2u);
}
TEST_F(ReplacementTest, FailAddInsertAtOtherInsert) {
@@ -187,38 +175,6 @@ TEST_F(ReplacementTest, FailAddInsertAtO
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) {
@@ -233,29 +189,6 @@ TEST_F(ReplacementTest, CanApplyReplacem
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");
More information about the cfe-commits
mailing list