[PATCH] D21547: Added calculateRangesAfterReplaments() to calculate affacted ranges in the new code.

[Daniel Jasper via cfe-commits]

djasper added a comment.

Nice :)


================
Comment at: include/clang/Tooling/Core/Replacement.h:62
@@ +61,3 @@
+  /// \brief Whether this range is less-than \p RHS or not.
+  bool operator<(const Range &RHS) const {
+    if (Offset != RHS.getOffset())
----------------
Can we just add a lambda that does this to the std::sort operation? I am not sure that this is actually a natural order for all Replacements.

================
Comment at: include/clang/Tooling/Core/Replacement.h:69
@@ +68,3 @@
+  /// \brief Whether this range equals to \p RHS or not.
+  bool operator==(const Range &RHS) const {
+    return Offset == RHS.getOffset() && Length == RHS.getLength();
----------------
Why is this required?

================
Comment at: lib/Tooling/Core/Replacement.cpp:287
@@ +286,3 @@
+  std::sort(Ranges.begin(), Ranges.end());
+  auto I = Ranges.begin();
+  unsigned CurBegin = I->getOffset();
----------------
maybe:

  std::vector<Range> Result;
  for (const auto &R: Ranges) {
    if (Result.empty() || !Result.back().overlapsWith(R)) {
      Result.push_back(R);
    } else {
      unsigned NewEnd = std::max(Result.back.getOffset() + Result.back.getLength(), R.getOffset() + R.getLength());
      Result[Result.size() - 1] = Range(Result.back.getOffset(), NewEnd - Result.back.getOffset());
    }
  }

================
Comment at: lib/Tooling/Core/Replacement.cpp:319
@@ +318,3 @@
+                                 const std::vector<Range> &Ranges) {
+  if (Ranges.empty())
+    return calculateChangedRanges(Replaces);
----------------
I'd probably leave these early exits out. I don't think anyone will ever notice the performance difference.

================
Comment at: lib/Tooling/Core/Replacement.cpp:321
@@ +320,3 @@
+    return calculateChangedRanges(Replaces);
+
+  if (Replaces.empty())
----------------
I think you need to do:

  Ranges = mergeAndSortRanges(Ranges);

here


http://reviews.llvm.org/D21547