Re: [PATCH] D24663: When replacements have the same offset, make replacements with smaller length order first in the set.

[Eric Liu via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL281819: When replacements have the same offset, make 
replacements with smaller length… (authored by ioeric).

Changed prior to commit:
  https://reviews.llvm.org/D24663?vs=71730=71731#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D24663

Files:
  cfe/trunk/include/clang/Tooling/Core/Replacement.h
  cfe/trunk/lib/Tooling/Core/Replacement.cpp

Index: cfe/trunk/lib/Tooling/Core/Replacement.cpp
===
--- cfe/trunk/lib/Tooling/Core/Replacement.cpp
+++ cfe/trunk/lib/Tooling/Core/Replacement.cpp
@@ -83,11 +83,8 @@
   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 @@
 
 bool applyAllReplacements(const Replacements , Rewriter ) {
   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 @@
   "", 0, llvm::MemoryBuffer::getMemBuffer(Code, ""));
   FileID ID = SourceMgr.createFileID(Files.getFile(""), 
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("", I->getOffset(), I->getLength(),
 I->getReplacementText());
 if (!Replace.apply(Rewrite))
Index: cfe/trunk/include/clang/Tooling/Core/Replacement.h
===
--- cfe/trunk/include/clang/Tooling/Core/Replacement.h
+++ cfe/trunk/include/clang/Tooling/Core/Replacement.h
@@ -151,6 +151,7 @@
 
  public:
   typedef ReplacementsImpl::const_iterator const_iterator;
+  typedef ReplacementsImpl::const_reverse_iterator const_reverse_iterator;
 
   Replacements() = default;
 
@@ -193,6 +194,10 @@
 
   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 ) const {
 return Replaces == RHS.Replaces;
   }


Index: cfe/trunk/lib/Tooling/Core/Replacement.cpp
===
--- cfe/trunk/lib/Tooling/Core/Replacement.cpp
+++ cfe/trunk/lib/Tooling/Core/Replacement.cpp
@@ -83,11 +83,8 @@
   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 @@
 
 bool applyAllReplacements(const Replacements , Rewriter ) {
   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 @@
   "", 0, llvm::MemoryBuffer::getMemBuffer(Code, ""));
   FileID ID = SourceMgr.createFileID(Files.getFile(""), 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("", I->getOffset(), I->getLength(),
 I->getReplacementText());
 if (!Replace.apply(Rewrite))
Index: cfe/trunk/include/clang/Tooling/Core/Replacement.h
===
--- cfe/trunk/include/clang/Tooling/Core/Replacement.h
+++ cfe/trunk/include/clang/Tooling/Core/Replacement.h
@@ -151,6 +151,7 @@
 
  public:
   typedef ReplacementsImpl::const_iterator const_iterator;
+  typedef ReplacementsImpl::const_reverse_iterator const_reverse_iterator;
 
   

Re: [PATCH] D24663: When replacements have the same offset, make replacements with smaller length order first in the set.

[Eric Liu via cfe-commits]

ioeric updated this revision to Diff 71730.
ioeric added a comment.

- Format code properly.


https://reviews.llvm.org/D24663

Files:
  include/clang/Tooling/Core/Replacement.h
  lib/Tooling/Core/Replacement.cpp

Index: lib/Tooling/Core/Replacement.cpp
===
--- lib/Tooling/Core/Replacement.cpp
+++ lib/Tooling/Core/Replacement.cpp
@@ -83,11 +83,8 @@
   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 @@
 
 bool applyAllReplacements(const Replacements , Rewriter ) {
   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 @@
   "", 0, llvm::MemoryBuffer::getMemBuffer(Code, ""));
   FileID ID = SourceMgr.createFileID(Files.getFile(""), 
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("", I->getOffset(), I->getLength(),
 I->getReplacementText());
 if (!Replace.apply(Rewrite))
Index: include/clang/Tooling/Core/Replacement.h
===
--- include/clang/Tooling/Core/Replacement.h
+++ include/clang/Tooling/Core/Replacement.h
@@ -151,6 +151,7 @@
 
  public:
   typedef ReplacementsImpl::const_iterator const_iterator;
+  typedef ReplacementsImpl::const_reverse_iterator const_reverse_iterator;
 
   Replacements() = default;
 
@@ -193,6 +194,10 @@
 
   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 ) const {
 return Replaces == RHS.Replaces;
   }


Index: lib/Tooling/Core/Replacement.cpp
===
--- lib/Tooling/Core/Replacement.cpp
+++ lib/Tooling/Core/Replacement.cpp
@@ -83,11 +83,8 @@
   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 @@
 
 bool applyAllReplacements(const Replacements , Rewriter ) {
   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 @@
   "", 0, llvm::MemoryBuffer::getMemBuffer(Code, ""));
   FileID ID = SourceMgr.createFileID(Files.getFile(""), 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("", I->getOffset(), I->getLength(),
 I->getReplacementText());
 if (!Replace.apply(Rewrite))
Index: include/clang/Tooling/Core/Replacement.h
===
--- include/clang/Tooling/Core/Replacement.h
+++ include/clang/Tooling/Core/Replacement.h
@@ -151,6 +151,7 @@
 
  public:
   typedef ReplacementsImpl::const_iterator const_iterator;
+  typedef ReplacementsImpl::const_reverse_iterator const_reverse_iterator;
 
   Replacements() = default;
 
@@ -193,6 +194,10 @@
 
   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 ) const {
 return Replaces == RHS.Replaces;
   }

Re: [PATCH] D24663: When replacements have the same offset, make replacements with smaller length order first in the set.

[Eric Liu via cfe-commits]

ioeric updated this revision to Diff 71729.
ioeric marked an inline comment as done.
ioeric added a comment.

- Use auto.


https://reviews.llvm.org/D24663

Files:
  include/clang/Tooling/Core/Replacement.h
  lib/Tooling/Core/Replacement.cpp

Index: lib/Tooling/Core/Replacement.cpp
===
--- lib/Tooling/Core/Replacement.cpp
+++ lib/Tooling/Core/Replacement.cpp
@@ -83,11 +83,8 @@
   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 @@
 
 bool applyAllReplacements(const Replacements , Rewriter ) {
   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,7 +431,8 @@
   "", 0, llvm::MemoryBuffer::getMemBuffer(Code, ""));
   FileID ID = SourceMgr.createFileID(Files.getFile(""), 
SourceLocation(),
  clang::SrcMgr::C_User);
-  for (Replacements::const_iterator I = Replaces.begin(), E = Replaces.end();
+  for (auto I = Replaces.rbegin(),
+E = Replaces.rend();
I != E; ++I) {
 Replacement Replace("", I->getOffset(), I->getLength(),
 I->getReplacementText());
Index: include/clang/Tooling/Core/Replacement.h
===
--- include/clang/Tooling/Core/Replacement.h
+++ include/clang/Tooling/Core/Replacement.h
@@ -151,6 +151,7 @@
 
  public:
   typedef ReplacementsImpl::const_iterator const_iterator;
+  typedef ReplacementsImpl::const_reverse_iterator const_reverse_iterator;
 
   Replacements() = default;
 
@@ -193,6 +194,10 @@
 
   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 ) const {
 return Replaces == RHS.Replaces;
   }


Index: lib/Tooling/Core/Replacement.cpp
===
--- lib/Tooling/Core/Replacement.cpp
+++ lib/Tooling/Core/Replacement.cpp
@@ -83,11 +83,8 @@
   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 @@
 
 bool applyAllReplacements(const Replacements , Rewriter ) {
   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,7 +431,8 @@
   "", 0, llvm::MemoryBuffer::getMemBuffer(Code, ""));
   FileID ID = SourceMgr.createFileID(Files.getFile(""), SourceLocation(),
  clang::SrcMgr::C_User);
-  for (Replacements::const_iterator I = Replaces.begin(), E = Replaces.end();
+  for (auto I = Replaces.rbegin(),
+E = Replaces.rend();
I != E; ++I) {
 Replacement Replace("", I->getOffset(), I->getLength(),
 I->getReplacementText());
Index: include/clang/Tooling/Core/Replacement.h
===
--- include/clang/Tooling/Core/Replacement.h
+++ include/clang/Tooling/Core/Replacement.h
@@ -151,6 +151,7 @@
 
  public:
   typedef ReplacementsImpl::const_iterator const_iterator;
+  typedef ReplacementsImpl::const_reverse_iterator const_reverse_iterator;
 
   Replacements() = default;
 
@@ -193,6 +194,10 @@
 
   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 ) const {
 return Replaces == 

Re: [PATCH] D24663: When replacements have the same offset, make replacements with smaller length order first in the set.

[Daniel Jasper via cfe-commits]

djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.

Looks good.



Comment at: lib/Tooling/Core/Replacement.cpp:407
@@ -409,3 +406,3 @@
   bool Result = true;
-  for (Replacements::const_iterator I = Replaces.begin(),
-E = Replaces.end();
+  for (Replacements::const_reverse_iterator I = Replaces.rbegin(),
+E = Replaces.rend();

Maybe use auto?


https://reviews.llvm.org/D24663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24663: When replacements have the same offset, make replacements with smaller length order first in the set.

[Eric Liu via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/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.


https://reviews.llvm.org/D24663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24663: When replacements have the same offset, make replacements with smaller length order first in the set.

[Manuel Klimek via cfe-commits]

klimek added a comment.

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


https://reviews.llvm.org/D24663



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits