<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Jun 26, 2014 at 6:32 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="">================<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>
</div><div class="">Manuel Klimek wrote:<br>
> I thought LLVM style was to not explicitly check against NULL? Also, nullptr?<br>
</div>This was just copy/paste from the original implementation before NULL had been cleaned up (presumably by Craig Topper) here. I've updated for this and a couple of other places that had been changed since I moved the code.<br>
<div class=""><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>
</div><div class="">Manuel Klimek wrote:<br>
> I'd change the name of things that do not return a new pointer to "create...".<br>
</div>Sure thing - I can do that renaming. I'll leave that until we've hammered out the rest (but I'll be sure to include it in the same commit). Should just be mechanical.<br>
<div class=""><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>
</div><div class="">Manuel Klimek wrote:<br>
> I actually dislike that change somewhat (it introduces more structural duplication).<br>
</div>Yep, I called this out as the only place that was "interesting" in the 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></blockquote><div><br></div><div>Sorry I missed that - that is what I meant with "taking away options from the user of the interface" (and I couldn't really put into words nicely). I agree that it doesn't matter too much, but on the other hand I still don't find the other changes to be that much of a benefit. I still think the whole thing is not worth the time to change it, even if it is a slight improvement.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
It could be refactored in a few other ways (like wrapping up a "run" 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></blockquote><div><br></div><div>Can we do the reverse and instead have a lambda that returns the factory?</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The next alternative would be to copy/move the factory into a dynamic 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></blockquote><div><br></div><div>If we have to do that, I'd argue we should stick with factories and pointers and not go to value objects at all.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
(well, sorry, all those examples are assuming the dtor becomes non-virtual - in this patch they're still virtual, so unique_ptr could be used instead of shared_ptr, but "newClone" would still be needed to move from static to a dynamic allocation)<br>
<br>
<a href="http://reviews.llvm.org/D4313" target="_blank">http://reviews.llvm.org/D4313</a><br>
<br>
<br>
</blockquote></div><br></div></div>