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

[Manuel Klimek via cfe-commits]

klimek added inline comments.

================
Comment at: include/clang/Tooling/Core/Replacement.h:237
@@ +236,3 @@
+/// related to the same file entry are put into the same vector.
+FileToReplacementsMap groupReplacementsByFile(const Replacements &Replaces,
+                                              FileManager &Files);
----------------
ioeric wrote:
> djasper wrote:
> > Hm... I am not sure here. I think I would implement this entirely without FileManager or FileEntries, just based on the names of the file. I guess you are worried about different paths leading to the same FileEntry?
> You are right. Getting rid of `FileManger` would make more sense for users. And yes, I was worrying about different names leading to the same entry. Do we need to worry about this case? Or we can assume Replacements for the same file always have the same file path? 
I like a map-based interface slightly better here, mainly because I think it's a bit harder to write buggy code against it, it's less work if the user wants to apply some kind of sharding (threads can just hand around the vectors without the need to loop over the vector yet again to split it, or hand indices around), and (to me at least) it is slightly easier to grasp the function's contract.

Regarding the FileEntries, I'm not overly worried about file entries leading to the same file (I believe this is not too hard to handle one level higher if necessary), and the interface would seem significantly simpler without the FileManager.

================
Comment at: include/clang/Tooling/ReplacementsFormat.h:40
@@ +39,3 @@
+                                   Rewriter &Rewrite,
+                                   const format::FormatStyle &Style);
+
----------------
djasper wrote:
> This is probably a pre-existing issue in formatReplacements, but I think conceptually it might be wrong to hand in the Style. I think it might be a better idea to use clang::format::getStyle() with the filename stored inside of the replacement to get the style that should actually be used for any given change. Not sure whether we'd also want the capability to overwrite this. Manuel, what do you think?
(not sure whether that's still unresolved, as I think I talked about this with Eric already)
I believe that is what style=file is for, right? I'm fine with a convenience method with style=file, but it seems like the generic one makes sense to have, too.


http://reviews.llvm.org/D17852