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

[Tim Shen via Phabricator via llvm-commits]

timshen added inline comments.


================
Comment at: llvm/include/llvm/ADT/function.h:9
+//===----------------------------------------------------------------------===//
+/// \file
+/// This file provides a collection of function (or more generally, callable)
----------------
Can we add a bit of future plan? Is it going to merge with the future-standard unique_function, if exist at all?


================
Comment at: llvm/include/llvm/ADT/function.h:54
+        sizeof(OutOfLineStorageT) <= InlineStorageSize,
+        "Should always use all of teh out-of-line storage for inline storage!");
+
----------------
teh duh!


================
Comment at: llvm/include/llvm/ADT/function.h:80
+  // us dispatch multiple different methods.
+  using CallbackPointerUnionT = PointerUnion<CallPtrT, NonTrivialCallbacks *>;
+
----------------
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.


================
Comment at: llvm/include/llvm/ADT/function.h:227
+
+  ReturnT operator()(ParamTs... Params) {
+    void *CallableAddr =
----------------
Do we want to perfect-forward the parameters?


Repository:
  rL LLVM

https://reviews.llvm.org/D48349