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

[Manuel Klimek]

On Thu, Jun 26, 2014 at 6:32 PM, David Blaikie <dblaikie at gmail.com> wrote:

> ================
> 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."
>

Sorry I missed that - that is what I meant with "taking away options from
the user of the interface" (and I couldn't really put into words nicely). I
agree that it doesn't matter too much, but on the other hand I still don't
find the other changes to be that much of a benefit. I still think the
whole thing is not worth the time to change it, even if it is a slight
improvement.


> 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.
>

Can we do the reverse and instead have a lambda that returns the factory?


> 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.
>

If we have to do that, I'd argue we should stick with factories and
pointers and not go to value objects at all.


> (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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140626/e23f37b1/attachment.html>