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 <[email protected]> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
