[PATCH] Own/produce FrontendActionFactories by value, rather than new'd/unique_ptr ownership.

[Manuel Klimek]

On Fri, Aug 8, 2014 at 7:54 PM, David Blaikie <dblaikie at gmail.com> wrote:

> 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);
>

I still think we already spent more time on this issue than it will ever
pay back one way or the other. I'd still be very very strongly in favor for
having a virtual dtor if we have an exposed interface we intend to be
extended, but I can see that expectations may change over time, so I'll not
argue any more and let's try it your way.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140808/93c0f510/attachment.html>