From 18e5245c5a5ae0867d04257b4b56540ac38aad24 Mon Sep 17 00:00:00 2001
From: Eric Liu <ioeric@google.com>
Date: Sat, 17 Sep 2016 12:26:42 +0000
Subject: [PATCH] When replacements have the same offset, make replacements
 with smaller length order first in the set.

Summary:
No behavioral change intended. The change makes iterating the replacements set more intuitive in Replacements class implementation. Previously, insertion is ordered before an deletion/replacement with the same offset, which is counter-intuitive for implementation, especially for a followup patch to support adding insertions around replacements.

With the current ordering, we only need to make `applyAllReplacements` iterate the replacements set reversely when applying them so that deletion/replacement is still applied before insertion with the same offset.

Reviewers: klimek, djasper

Subscribers: klimek, cfe-commits

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

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@281819 91177308-0d34-0410-b5e6-96231b3b80d8
---
 include/clang/Tooling/Core/Replacement.h |  5 +++++
 lib/Tooling/Core/Replacement.cpp         | 12 +++---------
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/include/clang/Tooling/Core/Replacement.h b/include/clang/Tooling/Core/Replacement.h
index 22fc2ae7a4..bf4b89e05d 100644
--- a/include/clang/Tooling/Core/Replacement.h
+++ b/include/clang/Tooling/Core/Replacement.h
@@ -151,6 +151,7 @@ class Replacements {
 
  public:
   typedef ReplacementsImpl::const_iterator const_iterator;
+  typedef ReplacementsImpl::const_reverse_iterator const_reverse_iterator;
 
   Replacements() = default;
 
@@ -193,6 +194,10 @@ class Replacements {
 
   const_iterator end() const { return Replaces.end(); }
 
+  const_reverse_iterator rbegin() const  { return Replaces.rbegin(); }
+
+  const_reverse_iterator rend() const { return Replaces.rend(); }
+
   bool operator==(const Replacements &RHS) const {
     return Replaces == RHS.Replaces;
   }
diff --git a/lib/Tooling/Core/Replacement.cpp b/lib/Tooling/Core/Replacement.cpp
index b257f0f137..655582369d 100644
--- a/lib/Tooling/Core/Replacement.cpp
+++ b/lib/Tooling/Core/Replacement.cpp
@@ -83,11 +83,8 @@ bool operator<(const Replacement &LHS, const Replacement &RHS) {
   if (LHS.getOffset() != RHS.getOffset())
     return LHS.getOffset() < RHS.getOffset();
 
-  // Apply longer replacements first, specifically so that deletions are
-  // executed before insertions. It is (hopefully) never the intention to
-  // delete parts of newly inserted code.
   if (LHS.getLength() != RHS.getLength())
-    return LHS.getLength() > RHS.getLength();
+    return LHS.getLength() < RHS.getLength();
 
   if (LHS.getFilePath() != RHS.getFilePath())
     return LHS.getFilePath() < RHS.getFilePath();
@@ -407,9 +404,7 @@ unsigned Replacements::getShiftedCodePosition(unsigned Position) const {
 
 bool applyAllReplacements(const Replacements &Replaces, Rewriter &Rewrite) {
   bool Result = true;
-  for (Replacements::const_iterator I = Replaces.begin(),
-                                    E = Replaces.end();
-       I != E; ++I) {
+  for (auto I = Replaces.rbegin(), E = Replaces.rend(); I != E; ++I) {
     if (I->isApplicable()) {
       Result = I->apply(Rewrite) && Result;
     } else {
@@ -436,8 +431,7 @@ llvm::Expected<std::string> applyAllReplacements(StringRef Code,
       "<stdin>", 0, llvm::MemoryBuffer::getMemBuffer(Code, "<stdin>"));
   FileID ID = SourceMgr.createFileID(Files.getFile("<stdin>"), SourceLocation(),
                                      clang::SrcMgr::C_User);
-  for (Replacements::const_iterator I = Replaces.begin(), E = Replaces.end();
-       I != E; ++I) {
+  for (auto I = Replaces.rbegin(), E = Replaces.rend(); I != E; ++I) {
     Replacement Replace("<stdin>", I->getOffset(), I->getLength(),
                         I->getReplacementText());
     if (!Replace.apply(Rewrite))
-- 
2.40.0