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

[Manuel Klimek]

================
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:
> Edwin Vane wrote:
> > 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?
> > With the three-method approach this point is moot but I'm still interested in the answer in general.
> 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 :)
Did you not see my answer for some reason? If so, I'll file a phab bug :)


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