<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jun 26, 2014 at 6:45 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Jun 26, 2014 at 10:37 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
> On Thu, Jun 26, 2014 at 6:32 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> ================<br>
>> Comment at: include/clang/Tooling/Tooling.h:320<br>
>> @@ +319,3 @@<br>
>> + return false;<br>
>> + if (Callbacks != NULL)<br>
>> + return Callbacks->handleBeginSource(CI, Filename);<br>
>> ----------------<br>
>> Manuel Klimek wrote:<br>
>> > I thought LLVM style was to not explicitly check against NULL? Also,<br>
>> > nullptr?<br>
>> This was just copy/paste from the original implementation before NULL had<br>
>> been cleaned up (presumably by Craig Topper) here. I've updated for this and<br>
>> a couple of other places that had been changed since I moved the code.<br>
>><br>
>> ================<br>
>> Comment at: include/clang/Tooling/Tooling.h:340<br>
>> @@ +339,3 @@<br>
>> +template <typename T><br>
>> +internal::SimpleFrontendActionFactory<T> newFrontendActionFactory() {<br>
>> + return internal::SimpleFrontendActionFactory<T>();<br>
>> ----------------<br>
>> Manuel Klimek wrote:<br>
>> > I'd change the name of things that do not return a new pointer to<br>
>> > "create...".<br>
>> Sure thing - I can do that renaming. I'll leave that until we've hammered<br>
>> out the rest (but I'll be sure to include it in the same commit). Should<br>
>> just be mechanical.<br>
>><br>
>> ================<br>
>> Comment at: tools/clang-check/ClangCheck.cpp:223<br>
>> @@ -229,1 +222,3 @@<br>
>> + return Tool.run(newFrontendActionFactory<FixItAction>());<br>
>> + return Tool.run(newFrontendActionFactory(&CheckFactory));<br>
>> }<br>
>> ----------------<br>
>> Manuel Klimek wrote:<br>
>> > I actually dislike that change somewhat (it introduces more structural<br>
>> > duplication).<br>
>> Yep, I called this out as the only place that was "interesting" in the<br>
>> original thread, but perhaps it got lost in all the words:<br>
>><br>
>> "The one caller that<br>
>> did:<br>
>><br>
>> unique_ptr<factory> p;<br>
>> if (x)<br>
>> p = newFactory();<br>
>> else if (y)<br>
>> p = newFactory(a);<br>
>> else<br>
>> p = newFactory(b);<br>
>> Tool.run(p);<br>
>><br>
>> and the change was to roll the "Tool.run" call up into the call site,<br>
>> which wasn't overly burdensome."<br>
><br>
><br>
> Sorry I missed that - that is what I meant with "taking away options from<br>
> the user of the interface" (and I couldn't really put into words nicely). I<br>
> agree that it doesn't matter too much, but on the other hand I still don't<br>
> find the other changes to be that much of a benefit. I still think the whole<br>
> thing is not worth the time to change it, even if it is a slight<br>
> improvement.<br>
><br>
>><br>
>> It could be refactored in a few other ways (like wrapping up a "run"<br>
>> function:<br>
>><br>
>> auto run = [&](const FrontendActionFactory& F) { Tool.run(F); };<br>
>> if (x)<br>
>> run(newFactory());<br>
>> else if (y)<br>
>> run(newFactory(a));<br>
>> else<br>
>> run(newFactory(b));<br>
>><br>
>> Which doesn't really seem worthwhile to me.<br>
><br>
><br>
> Can we do the reverse and instead have a lambda that returns the factory?<br>
<br>
</div></div>No, because they're different types.<br>
<div class=""><br>
>> The next alternative would be to copy/move the factory into a dynamic<br>
>> allocation with a type erased destruction, which might look something like:<br>
>><br>
>> template<typename T><br>
>> std::shared_ptr<T> newClone(T &&t) {<br>
>> return std::make_shared<T>(std::forward<T>(t));<br>
>> }<br>
>><br>
>> ...<br>
>><br>
>> // Use shared_ptr for type erased destruction<br>
>> std::shared_ptr<FrontendActionFactory> F;<br>
>> if (x)<br>
>> F = newClone(newFactory());<br>
>> else if (y)<br>
>> F = newClone(newFactory(a));<br>
>> else<br>
>> F = newClone(newFactory(b));<br>
>> Tool.run(*F);<br>
>><br>
>> Which also seems excessive to me.<br>
><br>
><br>
> If we have to do that, I'd argue we should stick with factories and pointers<br>
> and not go to value objects at all.<br>
<br>
</div>OK. I'll CC Richard in case he has any further arguments, otherwise<br>
I'll leave it alone.<br>
<br>
Richard, to come back to your original question "Why do we need to<br>
heap-allocate the FrontendActionFactory at all?", the above is the<br>
only case where the dynamic allocation is used/important. What do you<br>
reckon - any other ways to rewrite/redesign that more nicely? Fair<br>
reason to impose dynamic allocation/virtual dtors on all other users?<br>
<br>
(I suppose a 3rd option would be for this one client to just not use<br>
the factories and "new" the objects directly - using shared_ptr still<br>
if the dtor becomes protected/non-virtual - but I guess Manuel would<br>
still find that too high of a cost)<br></blockquote><div><br></div><div>So, while I find this slightly less good here, I don't think it's a big thing either way - so from my point of view feel free to check in your current version. My biggest problem is actually that I find both solutions so close together from an overall "goodness" factor (both have their pros and cons), so I'd rather not spend too much time arguing or searching for which solution is slightly more right any more.</div>
<div><br></div><div>That said, if Richard has an opinion one way or the other, I'd also say let's just take that and go with it :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
- David<br>
</font></span></blockquote></div><br></div></div>