================
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:
> Peter Collingbourne wrote:
> > 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.
> So,
> 1. after re-reading RefactoringTool I agree with you that run() shouldn't be
> virtual any more (in fact, I think we can just make it non-virtual now).
> 2. I still don't see the symmetry between run() (yes, runActions might be a
> better name) and buildAST - buildASTs doesn't run any actions, and it doesn't
> help me in running any actions; it seems completely unrelated to what the
> clang tool does (apart from that it might be implemented on top of buildASTs
>
> It nearly seems like we're missing an abstraction between ToolInvocation and
> ClangTool... Hmmm...
> 2. I still don't see the symmetry between run() (yes, runActions might be a
> better name) and buildAST - buildASTs doesn't run any actions, and it doesn't
> help me in running any actions; it seems completely unrelated to what the
> clang tool does (apart from that it might be implemented on top of buildASTs
Likewise, a Clang tool may be implemented on top of runActions() -- running
actions may only be a small part of what a given Clang tool does. I see
ClangTool more as a handy abstraction that lets you do a number of AST-related
things with a set of source file paths -- exactly the kind of thing that a
Clang tool may need to do.
> It nearly seems like we're missing an abstraction between ToolInvocation and
> ClangTool... Hmmm...
There could be a ToolInvocation-like thing that builds ASTs, but I think that's
outside the scope of this patch.
================
Comment at: lib/Tooling/Tooling.cpp:209
@@ -208,3 +251,2 @@
Compiler.createSourceManager(*Files);
- addFileMappingsTo(Compiler.getSourceManager());
----------------
Manuel Klimek wrote:
> Peter Collingbourne wrote:
> > 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.
> Hmmm... I remember doing this for a reason, but I don't remember the reason...
Well, the tests still pass. Could it be to do with ownership? As far as I can
tell, SourceManager continues to take ownership of the MemoryBuffers (see the
logic in InitializeFileRemapping in Frontend/InitPreprocessor.cpp). In any
case, ASTUnit clients need to provide the mappings in PreprocessorOptions.
================
Comment at: unittests/Tooling/ToolingTest.cpp:154
@@ -129,1 +153,3 @@
+ clang::tooling::ToolInvocation Invocation(Args, new SyntaxOnlyAction,
+ &*Files);
Invocation.mapVirtualFile("test.cpp", "#include <abc>\n");
----------------
Manuel Klimek wrote:
> .getPtr()
Done.
================
Comment at: unittests/Tooling/ToolingTest.cpp:173
@@ -146,1 +172,3 @@
+ clang::tooling::ToolInvocation Invocation(Args, new SyntaxOnlyAction,
+ &*Files);
Invocation.mapVirtualFile("test.cpp", "#include <abc>\n");
----------------
Manuel Klimek wrote:
> .getPtr()
Done.
http://llvm-reviews.chandlerc.com/D2097
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
