[PATCH] D24800: Merge conflicting replacements when they are order-independent.

[Manuel Klimek via cfe-commits]

klimek added inline comments.

================
Comment at: include/clang/Tooling/Core/Replacement.h:177-178
@@ +176,4 @@
+  ///   - are insertions at the same offset and applying them in either order
+  ///     has the same effect, i.e. X + Y = Y + X if one inserts text X and the
+  ///     other inserts text Y.
+  /// Examples:
----------------
ioeric wrote:
> klimek wrote:
> > This seems incorrect. What am I missing?
> Here is the discussion about this: https://reviews.llvm.org/D24717
I misunderstood :) Perhaps change to:
, i.e. if X + Y == Y + X when inserting X and Y respectively.

================
Comment at: include/clang/Tooling/Core/Replacement.h:242
@@ +241,3 @@
+  // If `R` and all existing replacements are order-indepedent, then merge it
+  // with `Replaces` and returns the merged replacements; otheriwse, returns an
+  // error.
----------------
typo: otheriwse

================
Comment at: lib/Tooling/Core/Replacement.cpp:162
@@ +161,3 @@
+    auto &Prev = NewReplaces.back();
+    auto PrevEnd = Prev.getOffset() + Prev.getLength();
+    if (PrevEnd < R.getOffset()) {
----------------
I'd not use auto for simple integer types.

================
Comment at: lib/Tooling/Core/Replacement.cpp:166
@@ +165,3 @@
+    } else {
+      assert(PrevEnd == R.getOffset());
+      Replacement NewR(
----------------
Add comment why we can assert this.

================
Comment at: lib/Tooling/Core/Replacement.cpp:179-181
@@ +178,5 @@
+Replacements::mergeIfOrderIndependent(const Replacement &R) const {
+  Replacements Rs(R);
+  Replacements ShiftedRs(getReplacementInChangedCode(R));
+  Replacements ShiftedReplaces;
+  for (const auto &Replace : Replaces)
----------------
These names are confusing me...


================
Comment at: lib/Tooling/Core/Replacement.cpp:184
@@ +183,3 @@
+    ShiftedReplaces.Replaces.insert(Rs.getReplacementInChangedCode(Replace));
+  auto MergeRs = merge(ShiftedRs);
+  auto MergeReplaces = Rs.merge(ShiftedReplaces);
----------------
This comes from a single replacement - why do we need to call merge?


https://reviews.llvm.org/D24800