From 40a68ac1713105d746b221d68ad7c617953b6a01 Mon Sep 17 00:00:00 2001 From: Eric Liu Date: Mon, 19 Sep 2016 08:40:42 +0000 Subject: [PATCH] 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 --- include/clang/Tooling/Core/Replacement.h | 18 +++-- lib/Tooling/Core/Replacement.cpp | 46 +++++++++---- unittests/Tooling/RefactoringTest.cpp | 88 ++++++++++++++++++++++-- 3 files changed, 128 insertions(+), 24 deletions(-) diff --git a/include/clang/Tooling/Core/Replacement.h b/include/clang/Tooling/Core/Replacement.h index bf4b89e05d..f7c32193c4 100644 --- a/include/clang/Tooling/Core/Replacement.h +++ b/include/clang/Tooling/Core/Replacement.h @@ -159,14 +159,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. diff --git a/lib/Tooling/Core/Replacement.cpp b/lib/Tooling/Core/Replacement.cpp index 655582369d..db5eea413d 100644 --- a/lib/Tooling/Core/Replacement.cpp +++ b/lib/Tooling/Core/Replacement.cpp @@ -134,6 +134,14 @@ 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()) @@ -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 // 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. + // `I` is the smallest iterator (after `R`) whose entry cannot overlap. // If that is begin(), there are no overlaps. if (I == Replaces.begin()) { Replaces.insert(R); @@ -175,16 +194,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 `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( - "New replacement:\n" + R.toString() + - "\nconflicts with existing replacement:\n" + I->toString(), - llvm::inconvertibleErrorCode()); + return makeConflictReplacementsError(R, *I); } namespace { diff --git a/unittests/Tooling/RefactoringTest.cpp b/unittests/Tooling/RefactoringTest.cpp index d5877ac2f9..b4c269d1f1 100644 --- a/unittests/Tooling/RefactoringTest.cpp +++ b/unittests/Tooling/RefactoringTest.cpp @@ -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"); @@ -506,6 +573,17 @@ TEST(Range, CalculateRangesOfReplacements) { EXPECT_TRUE(Ranges[1].getLength() == 22); } +TEST(Range, CalculateRangesOfInsertionAroundReplacement) { + Replacements Replaces = toReplacements( + {Replacement("foo", 0, 2, ""), Replacement("foo", 0, 0, "ba")}); + + std::vector Ranges = Replaces.getAffectedRanges(); + + EXPECT_EQ(1ul, Ranges.size()); + EXPECT_EQ(0u, Ranges[0].getOffset()); + EXPECT_EQ(2u, Ranges[0].getLength()); +} + TEST(Range, RangesAfterEmptyReplacements) { std::vector Ranges = {Range(5, 6), Range(10, 5)}; Replacements Replaces; -- 2.40.0