When replacements have the same offset, make replacements with smaller length order...

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

diff --git a/include/clang/Tooling/Core/Replacement.h b/include/clang/Tooling/Core/Replacement.h
index 22fc2ae7a4473b280ae1971038403217da7b4877..bf4b89e05d9533adee600c7aa801249b97ffa5ed 100644 (file)
--- 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 b257f0f137239644176eb4e5256de6b3ed62830d..655582369d65b0135bd6021642fa9fcb32e52289 100644 (file)
--- 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))