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

[David Blaikie]

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