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

[Manuel Klimek]

================
Comment at: include/clang/Rewrite/Core/Rewriter.h:61
@@ +60,3 @@
+  /// this function is the responsibility of the caller.
+  const char *getRewriteResult() const;
+
----------------
Edwin Vane wrote:
> 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.
I think the correct answer here depends on what one wants to do with the result :) But until we have a benchmark that shows that the copy there matters, I'd assume it's so much smaller than parsing C++ that it'll never matter...

================
Comment at: include/clang/Tooling/Refactoring.h:125
@@ -120,3 +124,3 @@
 /// processed.
-class RefactoringTool {
+class RefactoringTool : public ClangTool {
 public:
----------------
Edwin Vane wrote:
> 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?
Good point. Yep, let's change that to inheritance.

================
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 {
----------------
Edwin Vane wrote:
> 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?
Well, to me it depends on how it affects the design. I'm all for pulling out abstractions that make sense, especially when they are just implementation details.

But just not wanting something in the (private section of a class in the) header is for me not enough of an argument on its own.

Obviously all those are judgement calls where opinions and taste can differ, so I'm always open to a good argument that might change my mind in a specific case :)


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