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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
