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

[Daniel Jasper via cfe-commits]

djasper added inline comments.

================
Comment at: include/clang/Tooling/Core/Replacement.h:230
@@ +229,3 @@
+
+typedef std::map<const std::string, Replacements>
+    FileToReplacementsMap;
----------------
ioeric wrote:
> djasper wrote:
> > I think the key type in a map is always const, so no need for "const".
> I think "const" is needed since the `Entry` passed to map's `[]` operator is of type `const FileEntry *` in `FileToReplaces[Entry].push_back(Replace)`. The code didn't compile without the "const" qualifier.  
Well, now it's a string and the strings are copied anyway. I am pretty sure it will compile after removing "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);
 
----------------
ioeric wrote:
> djasper wrote:
> > 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?
> But I think the tradeoff of using `vector<Replacements>` is the insertion time since we'd need to index Replcements with the same filename for each insertion. Or maybe we can have a map from filename to vector index as a local variable and return a vector<Replacements>?  
Yes, that is what I would probably do, have a local map and return a vector to keep the interface cleaner. But lets see what Manuel thinks.


http://reviews.llvm.org/D17852