klimek added inline comments. ================ Comment at: include/clang/Tooling/Core/Replacement.h:177-178 @@ +176,4 @@ + /// - are insertions at the same offset and applying them in either order + /// has the same effect, i.e. X + Y = Y + X if one inserts text X and the + /// other inserts text Y. + /// Examples: ---------------- ioeric wrote: > klimek wrote: > > This seems incorrect. What am I missing? > Here is the discussion about this: https://reviews.llvm.org/D24717 I misunderstood :) Perhaps change to: , i.e. if X + Y == Y + X when inserting X and Y respectively.
================ Comment at: include/clang/Tooling/Core/Replacement.h:242 @@ +241,3 @@ + // If `R` and all existing replacements are order-indepedent, then merge it + // with `Replaces` and returns the merged replacements; otheriwse, returns an + // error. ---------------- typo: otheriwse ================ Comment at: lib/Tooling/Core/Replacement.cpp:162 @@ +161,3 @@ + auto &Prev = NewReplaces.back(); + auto PrevEnd = Prev.getOffset() + Prev.getLength(); + if (PrevEnd < R.getOffset()) { ---------------- I'd not use auto for simple integer types. ================ Comment at: lib/Tooling/Core/Replacement.cpp:166 @@ +165,3 @@ + } else { + assert(PrevEnd == R.getOffset()); + Replacement NewR( ---------------- Add comment why we can assert this. ================ Comment at: lib/Tooling/Core/Replacement.cpp:179-181 @@ +178,5 @@ +Replacements::mergeIfOrderIndependent(const Replacement &R) const { + Replacements Rs(R); + Replacements ShiftedRs(getReplacementInChangedCode(R)); + Replacements ShiftedReplaces; + for (const auto &Replace : Replaces) ---------------- These names are confusing me... ================ Comment at: lib/Tooling/Core/Replacement.cpp:184 @@ +183,3 @@ + ShiftedReplaces.Replaces.insert(Rs.getReplacementInChangedCode(Replace)); + auto MergeRs = merge(ShiftedRs); + auto MergeReplaces = Rs.merge(ShiftedReplaces); ---------------- This comes from a single replacement - why do we need to call merge? https://reviews.llvm.org/D24800 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits