[PATCH] Templatize tooling::deduplicate()

[Daniel Jasper]

  Do you have an actual use case for this? I can't come up with a good one where extra information would actually help. Also, does it make sense to make the deduplication generic and leave all the other functions unchanged? How would you apply a vector<SomeOtherReplacement>?

  On the other hand, if there is a good use case to use the "context" and the serialization format supports it, we could also consider just adding that string. After all and empty string should have a low overhead (compared to FilePath and ReplacementText).

  Another thing that we need to consider eventually is the concept of a replacement group. What I mean by that is that several replacements need to be applied atomically, i.e. either all of them or none of them. Imagine you want to migrate a for-loop. Now, you have one replacement for the actual "for (...)" and possibly one replacement for each usage of "something[i]". Now, one of the latter might conflict, e.g. due to macro usage. Now, we need to abandon the entire for-rewrite. I don't have firm grasp on how we are going to implement this, but I have the suspicion that dealing with the extra information would make things harder.


================
Comment at: lib/Tooling/Refactoring.cpp:91
@@ +90,3 @@
+bool operator<(const Replacement &LHS, const Replacement &RHS) {
+  if (LHS.getFilePath() != RHS.getFilePath())
+    return LHS.getFilePath() < RHS.getFilePath();
----------------
I'd keep the original order of comparisons. String comparisons are relatively expensive. Thus, I'd do that only if offset and length are both equal.

================
Comment at: include/clang/Tooling/Refactoring.h:129
@@ +128,3 @@
+/// \brief Less-than operator between two Replacements.
+bool operator<(const Replacement &LHS, const Replacement &RHS);
+
----------------
Is there a particular reason to move these out of the replacement class?


http://llvm-reviews.chandlerc.com/D1415