[cfe-commits] [PATCH] Allow RefactoringTool to write to memory instead of always to disk

[Edwin Vane]

  I like the three method approach. New patch coming soon.


================
Comment at: include/clang/Tooling/Refactoring.h:125
@@ -120,3 +124,3 @@
 /// processed.
-class RefactoringTool {
+class RefactoringTool : public ClangTool {
 public:
----------------
Manuel Klimek wrote:
> I'm opposed to this change :) If you want this, you'll need to sell me on the benefit - the default for me is "no inheritance".
In an upcoming patch, I need RefactoringTool to provide access to ClangTool's mapVirtualFile() function. I thought this change was worthwhile to make in this patch due to the fact that rewrites can now end up in memory and thus the mapping functionality is much more useful.

One could provide just another forwarding function in RefactoringTool that calls mapVirtualFile but I thought it was clearer and cleaner to just use inheritance since, at least to me, there is clearly a IS-A relationship between RefactoringTool and ClangTool. If I've understood the nature of RefactoringTool incorrectly let me know. Is there something about inheritance you're adverse to here?

================
Comment at: include/clang/Rewrite/Core/Rewriter.h:61
@@ +60,3 @@
+  /// this function is the responsibility of the caller.
+  const char *getRewriteResult() const;
+
----------------
Manuel Klimek wrote:
> I think this is unneeded. Instead, we can use a raw_string_ostream and call write() on it.
Ok. Given the FIXME that's in the source about avoiding copies I was trying to avoid unnecessary copies here.

================
Comment at: lib/Tooling/Refactoring.cpp:142
@@ +141,3 @@
+// saveRewrittenFiles() and getResults(), without cluttering Refactoring.h's
+// header with includes that are really only needed by the implementation.
+class RewriteHelper {
----------------
Manuel Klimek wrote:
> I think not putting something into the header is not a good reason to pull out an abstraction. I also generally don't like abstractions called "Helper" (I understand that you don't intend this to be an abstraction, but my point is that if there's not abstraction, I don't think it makes sense to pull one out just to reduce stuff in the header).
Alright. I'm still new to this and take what I read in the LLVM coding standards at face value. I'm referring to the "include as little as possible". But even from a design standpoint, is it really necessary to expose the types that are really only needed by the implementation to every user of Refactoring.h?


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