[PATCH] D23119: Fix quadratic runtime when adding items to tooling::Replacements.

[Manuel Klimek via cfe-commits]

klimek updated this revision to Diff 66651.
klimek added a comment.

Remove re-implementation of overlaps check.


https://reviews.llvm.org/D23119

Files:
  lib/Tooling/Core/Replacement.cpp
  unittests/Tooling/RefactoringTest.cpp

Index: unittests/Tooling/RefactoringTest.cpp
===================================================================
--- unittests/Tooling/RefactoringTest.cpp
+++ unittests/Tooling/RefactoringTest.cpp
@@ -115,6 +115,26 @@
   llvm::consumeError(std::move(Err));
 }
 
+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((bool)Err);
+  llvm::consumeError(std::move(Err));
+
+  Replaces.clear();
+  // Test overlap with an existing insertion.
+  Err = Replaces.add(Replacement("x.cc", 10, 0, "insert"));
+  EXPECT_TRUE(!Err);
+  llvm::consumeError(std::move(Err));
+  Err = Replaces.add(Replacement("x.cc", 10, 3, "replace"));
+  EXPECT_TRUE((bool)Err);
+  llvm::consumeError(std::move(Err));
+}
+
 TEST_F(ReplacementTest, CanApplyReplacements) {
   FileID ID = Context.createInMemoryFile("input.cpp",
                                          "line1\nline2\nline3\nline4");
Index: lib/Tooling/Core/Replacement.cpp
===================================================================
--- lib/Tooling/Core/Replacement.cpp
+++ lib/Tooling/Core/Replacement.cpp
@@ -138,25 +138,46 @@
 }
 
 llvm::Error Replacements::add(const Replacement &R) {
-  if (R.getOffset() != UINT_MAX)
-    for (auto Replace : Replaces) {
-      if (R.getFilePath() != Replace.getFilePath())
-        return llvm::make_error<llvm::StringError>(
-            "All replacements must have the same file path. New replacement: " +
-                R.getFilePath() + ", existing replacements: " +
-                Replace.getFilePath() + "\n",
-            llvm::inconvertibleErrorCode());
-      if (R.getOffset() == Replace.getOffset() ||
-          Range(R.getOffset(), R.getLength())
-              .overlapsWith(Range(Replace.getOffset(), Replace.getLength())))
-        return llvm::make_error<llvm::StringError>(
-            "New replacement:\n" + R.toString() +
-                "\nconflicts with existing replacement:\n" + Replace.toString(),
-            llvm::inconvertibleErrorCode());
-    }
+  // Check the file path.
+  if (!Replaces.empty() && R.getFilePath() != Replaces.begin()->getFilePath())
+    return llvm::make_error<llvm::StringError>(
+        "All replacements must have the same file path. New replacement: " +
+            R.getFilePath() + ", existing replacements: " +
+            Replaces.begin()->getFilePath() + "\n",
+        llvm::inconvertibleErrorCode());
+
+  // Special-case header insertions.
+  if (R.getOffset() == UINT_MAX) {
+    Replaces.insert(R);
+    return llvm::Error::success();
+  }
 
-  Replaces.insert(R);
-  return llvm::Error::success();
+  // This replacement cannot conflict with replacements that end before
+  // this replacement starts or start after this replacement ends.
+  // We also know that there currently are no overlapping replacements.
+  // Thus, we know that all replacements that start after the end of the current
+  // replacement cannot overlap.
+  Replacement AtEnd(R.getFilePath(), R.getOffset() + R.getLength(), 0, "");
+  auto I = Replaces.upper_bound(AtEnd);
+  // I is the smallest iterator whose entry cannot overlap.
+  // If that is begin(), there are no overlaps.
+  if (I == Replaces.begin()) {
+    Replaces.insert(R);
+    return llvm::Error::success();
+  }
+  --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())
+           .overlapsWith(Range(I->getOffset(), I->getLength()))) {
+    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());
 }
 
 namespace {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D23119.66651.patch
Type: text/x-patch
Size: 4073 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160803/f298dbe8/attachment.bin>