[PATCH] D17852: Added formatAndApplyAllReplacements that works on multiple files in libTooling.

Daniel Jasper via cfe-commits cfe-commits at lists.llvm.org
Fri Mar 4 13:56:19 PST 2016

djasper added inline comments.

Comment at: include/clang/Tooling/Core/Replacement.h:228
@@ -226,3 +227,3 @@
 /// \pre Replacements must be for the same file.
-calculateChangedRangesInFile(const tooling::Replacements &Replaces);
+std::vector<Range> calculateChangedRangesInFile(const Replacements &Replaces);
This was pre-existing, but I'd remove the "InFile" suffix. It doesn't seem to add any information.

Comment at: include/clang/Tooling/Core/Replacement.h:230
@@ +229,3 @@
+typedef std::map<const std::string, Replacements>
+    FileToReplacementsMap;
I think the key type in a map is always const, so no need for "const".

Comment at: include/clang/Tooling/Core/Replacement.h:235
@@ -229,1 +234,3 @@
+/// related to the same file entry are put into the same vector.
+FileToReplacementsMap groupReplacementsByFile(const Replacements &Replaces);
I also wonder whether this should really return a map. I find such maps in interfaces sometimes a bit difficult as they have some cost and the subsequent analyses might actually prefer a different container. How about instead just returning a vector<Replacements>? I mean the Replacements themselves already have the filename stored in them so the key in the map is redundant anyway.

Manuel, any thoughts?


More information about the cfe-commits mailing list