[PATCH] D21748: Implement tooling::Replacements as a class.

[Manuel Klimek via cfe-commits]

klimek added inline comments.

================
Comment at: include/clang/Tooling/Core/Replacement.h:160
@@ +159,3 @@
+  /// This returns true if the replacement is successfully inserted; otherwise,
+  /// it returns an llvm::Error, i.e. there is conflict between R and the
+  /// existing replacements or R's file path is different from the filepath of
----------------
a conflict

================
Comment at: include/clang/Tooling/Core/Replacement.h:163
@@ +162,3 @@
+  /// existing replacements. Callers must explicitly check the Error returned.
+  /// This disable users to add order-dependent replacements. To control the
+  /// order in which order-dependent replacements are applied, use merge({R})
----------------
This prevents users from adding order-dependent replacements.

================
Comment at: include/clang/Tooling/Core/Replacement.h:172-173
@@ +171,4 @@
+
+  // Merge \p Replaces with the current replacements with \p Replaces
+  // referring to code after applying the current replacements.
+  Replacements merge(const Replacements &Replaces) const;
----------------
That sentence doesn't parse...

================
Comment at: include/clang/Tooling/Core/Replacement.h:176
@@ +175,3 @@
+
+  // This is the same as the merge above except this merge a single replacement.
+  Replacements merge(const Replacement &R) const;
----------------
merges

================
Comment at: include/clang/Tooling/Core/Replacement.h:179
@@ +178,3 @@
+
+  // This returns the affected ranges in the changed code.
+  std::vector<Range> getAffectedRanges() const;
----------------
Here, above and below:
Don't start function comments with "This" or the function name - just leave it out, like this:
"Returns the affected ranges ..."

================
Comment at: lib/Tooling/Core/Replacement.cpp:300
@@ +299,3 @@
+Replacements Replacements::merge(const Replacement &R) const {
+  Replacements Rs;
+  llvm::consumeError(Rs.add(R));
----------------
I'd probably add a single-replacement constructor for convenience.

================
Comment at: lib/Tooling/Core/Replacement.cpp:306
@@ +305,3 @@
+// Merge and sort overlapping ranges in \p Ranges.
+static std::vector<Range> mergeAndSortRanges(std::vector<Range> Ranges) {
+  std::sort(Ranges.begin(), Ranges.end(),
----------------
So, this doesn't do the same as Replacements::merge, right? I think we're getting into a bit confusing terminology - perhaps we can find a better name for this?

================
Comment at: lib/Tooling/Core/Replacement.cpp:332
@@ +331,3 @@
+                                 const std::vector<Range> &Ranges) {
+  auto MergedRanges = mergeAndSortRanges(Ranges);
+  tooling::Replacements FakeReplaces;
----------------
This function could need a comment on how it actually works - I don't remember, and the function names called don't really give us a good explanation. I know this is just code you're moving, but I think we can make it a bit easier to understand while we're at it :)


http://reviews.llvm.org/D21748