On Thu, Jun 26, 2014 at 6:32 PM, David Blaikie <dblai...@gmail.com> wrote:
> ================
> Comment at: include/clang/Tooling/Tooling.h:320
> @@ +319,3 @@
> + return false;
> + if (Callbacks != NULL)
> + return Callbacks->handleBeginSource(CI, Filename);
> ----------------
> Manuel Klimek wrote:
> > I thought LLVM style was to not explicitly check against NULL? Also,
> nullptr?
> 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.
>
> ================
> Comment at: include/clang/Tooling/Tooling.h:340
> @@ +339,3 @@
> +template <typename T>
> +internal::SimpleFrontendActionFactory<T> newFrontendActionFactory() {
> + return internal::SimpleFrontendActionFactory<T>();
> ----------------
> Manuel Klimek wrote:
> > I'd change the name of things that do not return a new pointer to
> "create...".
> 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.
>
> ================
> Comment at: tools/clang-check/ClangCheck.cpp:223
> @@ -229,1 +222,3 @@
> + return Tool.run(newFrontendActionFactory<FixItAction>());
> + return Tool.run(newFrontendActionFactory(&CheckFactory));
> }
> ----------------
> Manuel Klimek wrote:
> > I actually dislike that change somewhat (it introduces more structural
> duplication).
> 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:
>
> "The one caller that
> did:
>
> unique_ptr<factory> p;
> if (x)
> p = newFactory();
> else if (y)
> p = newFactory(a);
> else
> p = newFactory(b);
> Tool.run(p);
>
> and the change was to roll the "Tool.run" call up into the call site,
> which wasn't overly burdensome."
>
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.
> It could be refactored in a few other ways (like wrapping up a "run"
> function:
>
> auto run = [&](const FrontendActionFactory& F) { Tool.run(F); };
> if (x)
> run(newFactory());
> else if (y)
> run(newFactory(a));
> else
> run(newFactory(b));
>
> Which doesn't really seem worthwhile to me.
>
Can we do the reverse and instead have a lambda that returns the factory?
> The next alternative would be to copy/move the factory into a dynamic
> allocation with a type erased destruction, which might look something like:
>
> template<typename T>
> std::shared_ptr<T> newClone(T &&t) {
> return std::make_shared<T>(std::forward<T>(t));
> }
>
> ...
>
> // Use shared_ptr for type erased destruction
> std::shared_ptr<FrontendActionFactory> F;
> if (x)
> F = newClone(newFactory());
> else if (y)
> F = newClone(newFactory(a));
> else
> F = newClone(newFactory(b));
> Tool.run(*F);
>
> Which also seems excessive to me.
>
If we have to do that, I'd argue we should stick with factories and
pointers and not go to value objects at all.
> (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)
>
> http://reviews.llvm.org/D4313
>
>
>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
