After reviewing (& making some improvements in r215215 and r215214) some memory
leak fixes made in r213851 I think it's rather valuable to change Tool::run to
take a reference to a FrontendActionFactory, rather than a pointer. (indeed the
memory leaks related to that API that were fixed in r213851 are also fixed by
my patch above)
And while I don't mind churning the APIs multiple times (switching to
references, then, potentially, switching to the factory functions returning by
value) & doing all the legwork internally & externally - it might be nice to
settle on it.
I can also split out the other part involving the SingleFrontendActionFactory
(and fixing FrontendActionFactory::create to be explicit about returning
ownership) that we discussed/were confused by for a while, etc...
================
Comment at: tools/clang-check/ClangCheck.cpp:223
@@ -229,1 +222,3 @@
+ return Tool.run(newFrontendActionFactory<FixItAction>());
+ return Tool.run(newFrontendActionFactory(&CheckFactory));
}
----------------
David Blaikie wrote:
> Manuel Klimek wrote:
> > I actually dislike that change somewhat (it introduces more structural
> > duplication).
> Yep, I called this out as the only place that was "interesting" in the
> original thread, but perhaps it got lost in all the words:
>
> "The one caller that
> did:
>
> unique_ptr<factory> p;
> if (x)
> p = newFactory();
> else if (y)
> p = newFactory(a);
> else
> p = newFactory(b);
> Tool.run(p);
>
> and the change was to roll the "Tool.run" call up into the call site,
> which wasn't overly burdensome."
>
> It could be refactored in a few other ways (like wrapping up a "run" function:
>
> auto run = [&](const FrontendActionFactory& F) { Tool.run(F); };
> if (x)
> run(newFactory());
> else if (y)
> run(newFactory(a));
> else
> run(newFactory(b));
>
> Which doesn't really seem worthwhile to me.
>
> The next alternative would be to copy/move the factory into a dynamic
> allocation with a type erased destruction, which might look something like:
>
> template<typename T>
> std::shared_ptr<T> newClone(T &&t) {
> return std::make_shared<T>(std::forward<T>(t));
> }
>
> ...
>
> // Use shared_ptr for type erased destruction
> std::shared_ptr<FrontendActionFactory> F;
> if (x)
> F = newClone(newFactory());
> else if (y)
> F = newClone(newFactory(a));
> else
> F = newClone(newFactory(b));
> Tool.run(*F);
>
> Which also seems excessive to me.
>
> (well, sorry, all those examples are assuming the dtor becomes non-virtual -
> in this patch they're still virtual, so unique_ptr could be used instead of
> shared_ptr, but "newClone" would still be needed to move from static to a
> dynamic allocation)
Another option would be to expose the types that these factory functions return
and then just have the code specify the template arguments explicitly in this
one case (old code provided in comments for comparison):
std::shared_ptr<FrontendActionFactory> F; // or std::unique_ptr if the dtors
remain virtual
if (x)
// F = newFrontendActionFactory<clang::ento::AnalysisAction>();
F =
std::make_shared<SimpleFrontendActionFactory<clang::ento::AnalysisAction>>();
else if (y)
// F = newFrontendActionFactory<FixItAction>();
F = std::make_shared<SimpleFrontendActionFactory<FixItAction>>();
else
// F = newFrontendActionFactory(&CheckFactory);
F =
std::make_shared<FrontendActionFactoryAdapter<clang_check::ClangCheckActionFactory>>();
Tool.run(*F);
http://reviews.llvm.org/D4313
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
