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

[Manuel Klimek]

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

> On Thu, Jun 26, 2014 at 10:37 AM, Manuel Klimek <klimek at google.com> wrote:
> > 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?
>
> No, because they're different types.
>
> >> 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.
>
> OK. I'll CC Richard in case he has any further arguments, otherwise
> I'll leave it alone.
>
> Richard, to come back to your original question "Why do we need to
> heap-allocate the FrontendActionFactory at all?", the above is the
> only case where the dynamic allocation is used/important. What do you
> reckon - any other ways to rewrite/redesign that more nicely? Fair
> reason to impose dynamic allocation/virtual dtors on all other users?
>
> (I suppose a 3rd option would be for this one client to just not use
> the factories and "new" the objects directly - using shared_ptr still
> if the dtor becomes protected/non-virtual - but I guess Manuel would
> still find that too high of a cost)
>

So, while I find this slightly less good here, I don't think it's a big
thing either way - so from my point of view feel free to check in your
current version. My biggest problem is actually that I find both solutions
so close together from an overall "goodness" factor (both have their pros
and cons), so I'd rather not spend too much time arguing or searching for
which solution is slightly more right any more.

That said, if Richard has an opinion one way or the other, I'd also say
let's just take that and go with it :)


>
> - David
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140626/81364a6b/attachment.html>