[PATCH] D48349: [ADT] Add llvm::unique_function which is like std::function but supporting move-only closures.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc marked 3 inline comments as done.
chandlerc added a comment.

Thanks for the comments! Some responses below...



================
Comment at: llvm/include/llvm/ADT/function.h:80
+  // us dispatch multiple different methods.
+  using CallbackPointerUnionT = PointerUnion<CallPtrT, NonTrivialCallbacks *>;
+
----------------
timshen wrote:
> What's the rationale of choosing this specific data layout? Currently it's
>     tagged_union { CallPtrT, struct { CallPtrT, MovePtrT, DestroyPtrT }* }
> What makes us want it instead of, for example:
>     struct { CallPtrT, /* nullable */ struct { MovePtrT, DestroyPtrT}* }
> Or even:
>     // MoveOrDestroyPtrT does either Move or Destroy depending on an extra parameter.
>     struct { CallPtrT, /* nulltable */ MoveOrDestroyPtrT }
> ?
> 
> I'm not saying the examples are better, but just want more details on the decision.
The goal is to minimize the footprint by only using a single pointer within the object. Improved the comment to include this and not reference the prior design of a base class.


================
Comment at: llvm/include/llvm/ADT/function.h:187
+    ~unique_function();
+    new (this) unique_function(std::move(RHS));
+    return *this;
----------------
dberris wrote:
> Do you still need the `std::move(...)` here?
Yeah, within the function its an L-value reference and so this will copy otherwise. Same reason you need std::forward w/ a forwarding reference.


Repository:
  rL LLVM

https://reviews.llvm.org/D48349