[patch] FrontendActionFactory::create return by unique_ptr and some related cleanup

[Manuel Klimek]

Would you mind uploading them to citc or create a phab patch? I can't
really handle normal patch files that size without significant overhead.

On Thu Sep 18 2014 at 11:05:46 PM David Blaikie <dblaikie at gmail.com> wrote:

> Splitting some of this off from the "Own/produce FrontendActionFactories
> by value, rather than new'd/unique_ptr ownership"
>
> This is just to change the FrontendActionFactory::create to return by
> unique_ptr.
>
> But I did find a bunch of verbose and repetitious code (creating
> FrontendActionFactories and FrontendActions) that could be greatly
> simplified with some utilities and lambdas. This would be simpler if the
> other FrontendActionFactor utility functions were returning by value too.
>
> Anyway, bunch of things between this patch and the other:
>
> 1) Just the FrontendActionFactory::create -> unique_ptr change (propagates
> to runToolOnCode* functions needing to take by unique_ptr too, as
> previously discussed)
> 2) More FrontendActionFactory helpers with lambdas (these could use real
> names - they just have placeholders at the moment)
> 3) Tool::run pass FrontendActionFactory by reference
> 4) Move existing FrontendActionFactory helpers to return by value (new* ->
> make* rename, probably)
>
> 1 and 2 are in these patches (but can be separated and committed in either
> order - or one/the other taken while dropping the other)
>
> 3 and 4 are in the other review, again - can be separated and
> accepted/declined
>
> But generally all 4 would work best together.
>
> (1) without (2) is more churn on every caller - when I could just tidy up
> most callers (most of the google-internal callers too -
> https://github.com/google/souper/blob/master/lib/ClangTool/Actions.cpp#L76
> is the only google-internal one that needs the "less trivial" flavor - all
> the other internal FrontendActionFactory subclasses can be removed and
> replaced with makeTrivialFrontendActionFactory).
>
> (3) makes (1) and (2) easier - moving to more return-by-value means some
> callers can't be easily migrated (see clang-modernize - which has a factory
> producing unique_ptr<FrontendActionFactory> and callers that
> "Tool.run(factory().get())" - until run takes a reference, that can't be
> migrated to return by value without having to introduce extra vsariables at
> the caller.
>
> (4) would make those builder/factory/helper functions consistent with (2)
> and avoid the need for dynamic allocation. Only one caller would be
> inconvenienced by this, it's simpler for everyone else (and less subtle
> code: "Tool.run(*newFrontendActionFactory(...))" - wait, does
> newFrontendActionFactory return a raw owning pointer? Did this just leak?)
>
> Sorry about going around and around with all this - hopefully this is a
> reasonable summary of some of the interdependencies/benefits... I can try
> to write something more structured/explicit if it'd be helpful. Happy to
> split/apply these in whatever parts you see fit.
>

Without having looked at the text patches, I think we want (3) (make Run
take by reference and non-owning), and then my approach would be:
I.) if possible normal code should not need to call new / make factories,
it should be able to just use classes (perhaps we can play some tricks with
templated constructors?)
II.) if we cannot do that, have new...() functions that return a unique
pointer
III.) if somebody sent me Tool.run(*newXXX()) in review I'd likely ask them
to pull out a local variable; yes, that's one line more, but much clearer;
that's what in my opinion unique pointer is for

I still find by-value non-ideal for 2 reasons:
1. it limits the clients (as you agreed) in some cases, thus having
non-zero  cost
2. I don't see that it has a real benefit when used with interfaces; can
you point me at bugs this would prevent? I still find that pattern highly
unexpected.

Perhaps this will be best resolved in a heated discussion when I visit the
US next month :)

Cheers,
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140919/a205c530/attachment.html>