<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 19, 2014 at 7:59 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">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></blockquote><div><br></div><div>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?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><br><div class="gmail_quote"><div><div>On Thu Sep 18 2014 at 11:05:46 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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></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></blockquote><div><br></div><div>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)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="gmail_quote"><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? </div></div></blockquote><div><br></div><div>Yep, can't think of any particular bugs it would prevent off-hand. Just seems generally simpler for the common case:<br><br>Tool.run(makeFrontendActionFactory(...), ...);<br><br>(& once I upload the changes into Phab so you can see them, you'll see the lambda utilities I've added with similar functionality)<br><br>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="gmail_quote"><div>I still find that pattern highly unexpected.</div></div></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="gmail_quote"><div>Perhaps this will be best resolved in a heated discussion when I visit the US next month :)<br></div></div></blockquote><div><br></div><div>Quite possibly (:</div></div></div></div>