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.<br><br><div class="gmail_quote">On Thu Sep 18 2014 at 11:05:46 PM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Splitting some of this off from the "Own/produce FrontendActionFactories by value, rather than new'd/unique_ptr ownership" <br><br>This is just to change the FrontendActionFactory::create to return by unique_ptr.<br><br>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. <br><br>Anyway, bunch of things between this patch and the other:<br><br>1) Just the FrontendActionFactory::create -> unique_ptr change (propagates to runToolOnCode* functions needing to take by unique_ptr too, as previously discussed)<br>2) More FrontendActionFactory helpers with lambdas (these could use real names - they just have placeholders at the moment)<br>3) Tool::run pass FrontendActionFactory by reference<br>4) Move existing FrontendActionFactory helpers to return by value (new* -> make* rename, probably)<br><br>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)<br><br>3 and 4 are in the other review, again - can be separated and accepted/declined<br><br>But generally all 4 would work best together.<br><br>(1) without (2) is more churn on every caller - when I could just tidy up most callers (most of the google-internal callers too - <a href="https://github.com/google/souper/blob/master/lib/ClangTool/Actions.cpp#L76" target="_blank">https://github.com/google/souper/blob/master/lib/ClangTool/Actions.cpp#L76</a> 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). <br><br>(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.<br><br>(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?)<br><br>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.</div></blockquote><div><br></div><div>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:</div><div>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?)</div><div>II.) if we cannot do that, have new...() functions that return a unique pointer</div><div>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</div><div><br></div><div>I still find by-value non-ideal for 2 reasons:</div><div>1. it limits the clients (as you agreed) in some cases, thus having non-zero  cost</div><div>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.</div><div><br></div><div>Perhaps this will be best resolved in a heated discussion when I visit the US next month :)</div><div><br></div><div>Cheers,</div><div>/Manuel</div><div> </div></div>