================
Comment at: include/clang/Tooling/Tooling.h:79
@@ -63,1 +78,3 @@
+ bool runInvocation(clang::CompilerInvocation *Invocation,
+ FileManager *Files);
----------------
Manuel Klimek wrote:
> Does doxygen default to having the base class' comment here?
It does. I've added a more informative comment.
================
Comment at: include/clang/Tooling/Tooling.h:275
@@ -225,1 +274,3 @@
+ /// append them to ASTs.
+ int buildASTs(std::vector<ASTUnit *> &ASTs);
----------------
Manuel Klimek wrote:
> Hm. ClangTool is made to be extended, for example by RefactoringClangTool.
> I'm unsure how buildAST fits that interface. I've not read the rest of the
> patch yet, but it's unclear to me how buildAST here is related to run() from
> the comment. If it's not, why is it in ClangTool? If it is, I think we need
> to explain it here...
> Hm. ClangTool is made to be extended, for example by RefactoringClangTool.
> I'm unsure how buildAST fits that interface.
I think it may make sense for classes like RefactoringTool to derive from
ClangTool as (I believe) RefactoringTool needs the ASTs it works with to use
the same FileManager (i.e. it can't just use any old AST). If that were not
the case I would be arguing that the functionality in RefactoringTool should
exist independently of ClangTool.
But I don't think it makes sense to allow run() to be overridden. The purpose
of run(), as I see it, is simple: it runs an action over a specified list of
source files. That is not necessarily the same thing as running a tool; a tool
may need to do something before or after or instead of running the actions, but
those things could just as easily be done by the caller. (In fact, I don't
think this function should be called run() at all; that name implies that all
Clang tools do all their work inside the run() function, which is not
necessarily always true. A better name may be something like runActions().)
It would make even less sense to me to allow buildASTs() to be overridden. In
the case where a refactoring tool uses buildASTs it will probably want to build
the ASTs, do some work and then apply the replacements. That strongly implies
to me that the functionality for applying replacements should live in a
separate member function in RefactoringTool. (runAndSave() could still exist,
but as a convenience function.)
> I've not read the rest of the patch yet, but it's unclear to me how buildAST
> here is related to run() from the comment. If it's not, why is it in
> ClangTool? If it is, I think we need to explain it here...
I see run and buildASTs as the two alternative ways to build a Clang tool: one
callback-based and the other value-based. (buildASTs happens to be implemented
using run, but that's not too important.) To that end, I think the tooling
interface should have a buildASTs* function for each run* function. If you
agree, I'll update LibTooling.rst with this information.
================
Comment at: lib/Tooling/Tooling.cpp:118
@@ +117,3 @@
+ Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+ Args.push_back(FileName.data());
+ return Args;
----------------
Manuel Klimek wrote:
> .str()? I don't think .data() is always 0-terminated.
Yeah. I didn't look too closely at the code I was factoring out but of course
StringRef-formed-from-toNullTerminatedStringRef has different invariants from
plain StringRef. Fixed.
================
Comment at: lib/Tooling/Tooling.cpp:159
@@ +158,3 @@
+public:
+ SingleFrontendActionFactory(FrontendAction *FAction) : FAction(FAction) {}
+
----------------
Manuel Klimek wrote:
> Can we %s/FAction/Action/? I don't think the F gives me anything.
Done.
================
Comment at: lib/Tooling/Tooling.cpp:198-200
@@ -197,5 +243,1 @@
- // ToolAction can have lifetime requirements for Compiler or its members, and
- // we need to ensure it's deleted earlier than Compiler. So we pass it to an
- // OwningPtr declared after the Compiler variable.
- OwningPtr<FrontendAction> ScopedToolAction(ToolAction.take());
----------------
Manuel Klimek wrote:
> Did you delete this comment intentionally?
I decided to remove it because I thought it only made sense in the context of
the former ToolInvocation which had ToolAction as a member. But upon a
rereading I think it makes sense to bring it back. Done.
================
Comment at: lib/Tooling/Tooling.cpp:209
@@ -208,3 +251,2 @@
Compiler.createSourceManager(*Files);
- addFileMappingsTo(Compiler.getSourceManager());
----------------
Manuel Klimek wrote:
> Huh? Why can we delete this?
The remappings are now created on the PreprocessorOptions on lines 213-219
instead of on the SourceManager directly.
================
Comment at: lib/Tooling/Tooling.cpp:130
@@ -119,1 +129,3 @@
+ ToolInvocation Invocation(getSyntaxOnlyToolArgs(Args, FileNameRef),
ToolAction,
+ &*Files);
----------------
Manuel Klimek wrote:
> I'd use getPtr().
Done.
================
Comment at: lib/Tooling/Tooling.cpp:339
@@ -311,3 +338,3 @@
});
- ToolInvocation Invocation(CommandLine, ActionFactory->create(), &Files);
+ ToolInvocation Invocation(CommandLine, Action, &*Files);
for (int I = 0, E = MappedFileContents.size(); I != E; ++I) {
----------------
Manuel Klimek wrote:
> I'd use getPtr().
Done.
http://llvm-reviews.chandlerc.com/D2097
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
