From: Artem Belevich Date: Wed, 14 Sep 2016 23:03:06 +0000 (+0000) Subject: Revert r281457 "Supports adding insertion around non-insertion replacements." X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=e119918940e135d30c11a07061dd3b0d516c3c90;p=clang Revert r281457 "Supports adding insertion around non-insertion replacements." Commit was breaking our internal tests. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@281557 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Tooling/Core/Replacement.h b/include/clang/Tooling/Core/Replacement.h index 9c177b783c..22fc2ae7a4 100644 --- a/include/clang/Tooling/Core/Replacement.h +++ b/include/clang/Tooling/Core/Replacement.h @@ -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. diff --git a/lib/Tooling/Core/Replacement.cpp b/lib/Tooling/Core/Replacement.cpp index d74c56f947..b257f0f137 100644 --- a/lib/Tooling/Core/Replacement.cpp +++ b/lib/Tooling/Core/Replacement.cpp @@ -137,14 +137,6 @@ void Replacement::setFromSourceRange(const SourceManager &Sources, ReplacementText); } -llvm::Error makeConflictReplacementsError(const Replacement &New, - const Replacement &Existing) { - return llvm::make_error( - "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 Replacement &R) { // 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 Replacement &R) { --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( + "New replacement:\n" + R.toString() + + "\nconflicts with existing replacement:\n" + I->toString(), + llvm::inconvertibleErrorCode()); } namespace { diff --git a/unittests/Tooling/RefactoringTest.cpp b/unittests/Tooling/RefactoringTest.cpp index acec55642c..d5877ac2f9 100644 --- a/unittests/Tooling/RefactoringTest.cpp +++ b/unittests/Tooling/RefactoringTest.cpp @@ -115,16 +115,15 @@ TEST_F(ReplacementTest, FailAddReplacements) { 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, AddAdjacentInsertionAndReplacement) { 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, FailAddRegression) { 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, 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) { @@ -233,29 +189,6 @@ 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");