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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
