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