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

[Manuel Klimek]

 On Fri, Sep 19, 2014, 20:27 David Blaikie <dblaikie at gmail.com> wrote:

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?

 If you can create a single patch you can upload it to phab. Phab doesn't
know about projects at that level.
Otherwise 2 reviews are also fine.




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)

 I think we agree. My argument was from an overall cost / benefit
perspective which I still think applies : my argument is that we can make
the more uncommon cases cheaper for users without making the common cases
worse.



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(...), ...);

 I'd still hope we can make the common case be :
SomeRealFAF Factory(...);
tool.run(Factory);

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

 I'll take a look at these. Thx for digging them up.



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/20140920/1848f9d4/attachment.html>