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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits