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

[ioeric]

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.

[klimek]

Test? Why are we doing this (should go into the CL description)?

[ioeric]

In D24663#544993, @klimek wrote:

Test? Why are we doing this (should go into the CL description)?

There is no behavioral change intended, and the goal here is not to break any test. A test for this change can be having an insertion and a deletion at the same offset, but this is not supported yet and will a followup patch.

[djasper]

Looks good.

lib/Tooling/Core/Replacement.cpp

407 ↗

(On Diff #71632)

Maybe use auto?

[ioeric]

• Use auto.

[ioeric]

• Format code properly.

cfe/trunk/include/clang/Tooling/Core/Replacement.h

 /// Two replacements are considered conflicts if they overlap or have the same
 /// offset (i.e. order-dependent).
 class Replacements {
  private:
    typedef std::set<Replacement> ReplacementsImpl;
 
  public:
   typedef ReplacementsImpl::const_iterator const_iterator;
+  typedef ReplacementsImpl::const_reverse_iterator const_reverse_iterator;
 
   Replacements() = default;
 
   explicit Replacements(const Replacement &R) { Replaces.insert(R); }
 
   /// \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,
public:
   void clear() { Replaces.clear(); }
 
   bool empty() const { return Replaces.empty(); }
 
   const_iterator begin() const { return Replaces.begin(); }
 
   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;
   }
 
 
 private:
   Replacements(const_iterator Begin, const_iterator End)
       : Replaces(Begin, End) {}

cfe/trunk/lib/Tooling/Core/Replacement.cpp

Stream << FilePath << ": " << ReplacementRange.getOffset() << ":+"
          << ReplacementRange.getLength() << ":\"" << ReplacementText << "\"";
   return Stream.str();
 }
 
 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();
   return LHS.getReplacementText() < RHS.getReplacementText();
 }
 
 bool operator==(const Replacement &LHS, const Replacement &RHS) {
   return LHS.getOffset() == RHS.getOffset() &&
for (const auto& R : Replaces) {
     }
     break;
   }
   return Position + Offset;
 }
 
 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 {
       Result = false;
     }
   }
   return Result;
 }
DiagnosticsEngine Diagnostics(
       IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs),
       new DiagnosticOptions);
   SourceManager SourceMgr(Diagnostics, Files);
   Rewriter Rewrite(SourceMgr, LangOptions());
   InMemoryFileSystem->addFile(
       "<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))
       return llvm::make_error<llvm::StringError>(
           "Failed to apply replacement: " + Replace.toString(),
           llvm::inconvertibleErrorCode());
   }
   std::string Result;