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

[David Blaikie]

================
Comment at: include/clang/Tooling/Tooling.h:320
@@ +319,3 @@
+        return false;
+      if (Callbacks != NULL)
+        return Callbacks->handleBeginSource(CI, Filename);
----------------
Manuel Klimek wrote:
> I thought LLVM style was to not explicitly check against NULL? Also, nullptr?
This was just copy/paste from the original implementation before NULL had been cleaned up (presumably by Craig Topper) here. I've updated for this and a couple of other places that had been changed since I moved the code.

================
Comment at: include/clang/Tooling/Tooling.h:340
@@ +339,3 @@
+template <typename T>
+internal::SimpleFrontendActionFactory<T> newFrontendActionFactory() {
+  return internal::SimpleFrontendActionFactory<T>();
----------------
Manuel Klimek wrote:
> I'd change the name of things that do not return a new pointer to "create...".
Sure thing - I can do that renaming. I'll leave that until we've hammered out the rest (but I'll be sure to include it in the same commit). Should just be mechanical.

================
Comment at: tools/clang-check/ClangCheck.cpp:223
@@ -229,1 +222,3 @@
+    return Tool.run(newFrontendActionFactory<FixItAction>());
+  return Tool.run(newFrontendActionFactory(&CheckFactory));
 }
----------------
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)

http://reviews.llvm.org/D4313