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

[David Blaikie]

On Fri, Sep 19, 2014 at 7:59 AM, Manuel Klimek <klimek at google.com> wrote:

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

Sure - though is there any nice way for me to put the clang and
clang-tools-extra changes in one place for review? Or should I just make
two Phab reviews?


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

Agreed (& I'll mince words slightly - it doesn't make the uncommon clients
impossible - there are several ways to address them even if we both make
the factory's non-dynamic and the dtor protected/non-virtual (the minor
duplication of "Tool.run(", or the use of something like shared_ptr (or
unique_ptr if we kept the dtor virtual) and a one-liner (4 lines) template
that creates a unique_ptr<T> from a T) - this is an important distinction,
like C++: Make the defaults simple and sufficiently efficient, while not
making the hard cases impossible)


> 2. I don't see that it has a real benefit when used with interfaces; can
> you point me at bugs this would prevent?
>

Yep, can't think of any particular bugs it would prevent off-hand. Just
seems generally simpler for the common case:

Tool.run(makeFrontendActionFactory(...), ...);

(& once I upload the changes into Phab so you can see them, you'll see the
lambda utilities I've added with similar functionality)

Less dynamic allocation, less possible null pointers (OK, so that's one
type of bug it would prevent - by not baking null-ness into the APIs it's
on the user if they end up dabbling in null pointers. Makes it harder to
write this bug: unique_ptr<FrontendActionFactory> u; /* bunch of
conditionals and I think I initialized 'u' in here somewhere */
Tool.run(*u); /* but I didn't and now < that crashes */ ), less dynamic
state to think about.


> I still find that pattern highly unexpected.
>

The pattern of a type with virtual functions and a non-virtual dtor? Yeah,
not exactly /common/ but we've had at least a couple of instances of this
arise organically in LLVM since we made the clang warning able to
differentiate the benign case using C++11 final. See LLVM commits r216170,
r217871, r217990.


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

Quite possibly (:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140919/02d9e861/attachment.html>