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

[Daniel Jasper via cfe-commits]

djasper added inline comments.

================
Comment at: include/clang/Tooling/Core/Replacement.h:68
@@ +67,3 @@
+/// \brief Less-than operator between two Ranges.
+bool operator<(const Range &LHS, const Range &RHS);
+
----------------
Can we keep these private for now?

================
Comment at: include/clang/Tooling/Core/Replacement.h:237
@@ +236,3 @@
+/// \brief Calculates the new ranges after \p Replaces are applied. These
+/// include both the original \p Ranges and the affected ranges of \p Replaces
+/// in the new code.
----------------
Should we make the latter optional?

================
Comment at: include/clang/Tooling/Core/Replacement.h:240
@@ +239,3 @@
+///
+/// \pre Replacemments must be for the same file.
+///
----------------
s/Replacemments/Replacements/

================
Comment at: include/clang/Tooling/Core/Replacement.h:245
@@ +244,3 @@
+std::vector<Range>
+calculateRangesAfterReplacements(const Replacements &Replaces,
+                                 std::vector<Range> Ranges);
----------------
I'd call this getShiftedRanges().

================
Comment at: include/clang/Tooling/Core/Replacement.h:246
@@ +245,3 @@
+calculateRangesAfterReplacements(const Replacements &Replaces,
+                                 std::vector<Range> Ranges);
+
----------------
const vector&?

================
Comment at: lib/Tooling/Core/Replacement.cpp:39
@@ +38,3 @@
+bool operator==(const Range &LHS, const Range &RHS) {
+  return (LHS.getOffset() == RHS.getOffset()) &&
+         (LHS.getLength() == RHS.getLength());
----------------
No need for parentheses.

================
Comment at: lib/Tooling/Core/Replacement.cpp:304
@@ -292,1 +303,3 @@
 
+// Returns a sorted ranges after merging all overlapping ranges.
+static void mergeAndSortRanges(std::vector<Range> &Ranges) {
----------------
Remove " a"

================
Comment at: lib/Tooling/Core/Replacement.cpp:305
@@ +304,3 @@
+// Returns a sorted ranges after merging all overlapping ranges.
+static void mergeAndSortRanges(std::vector<Range> &Ranges) {
+  if (Ranges.empty())
----------------
Just return the new vector and use a const parameter.

================
Comment at: lib/Tooling/Core/Replacement.cpp:328
@@ +327,3 @@
+std::vector<Range>
+calculateRangesAfterReplacements(const Replacements &Replaces,
+                                 std::vector<Range> Ranges) {
----------------
Couldn't we implement this using mergeReplacements?

Fundamentally:
- Turn Ranges into Replacements at (offset, length) with an empty (unimportant) replacement text of length "length".
- Merge these into Replaces
- Convert the resulting Replacements into Ranges, each with the Replacement's (offset, replacement_text.size()) and shifting subsequent ones forward.


http://reviews.llvm.org/D21547