[PATCH] D82581: [ADT] Support const-qualified unique_functions

[Kadir Cetinkaya via Phabricator via llvm-commits]

kadircet added inline comments.


================
Comment at: llvm/include/llvm/ADT/FunctionExtras.h:131
 
-  CallPtrT getTrivialCallback() const {
-    return CallbackAndInlineFlag.getPointer().template get<TrivialCallback *>()->CallPtr;
----------------
nit: let's keep this and make use of it in `getCallPtr` instead of inlining.


================
Comment at: llvm/include/llvm/ADT/FunctionExtras.h:158
+  CallPtrT getCallPtr() const {
+    return this->isTrivialCallback() ? CallbackAndInlineFlag.getPointer()
+                                           .template get<TrivialCallback *>()
----------------
nit: drop `this` (also the one before `getNonTriivialCallbacks()`)?


================
Comment at: llvm/include/llvm/ADT/FunctionExtras.h:171
+  void *getCalleePtr() const {
+    return this->isInlineStorage() ? this->getInlineStorage()
+                                   : this->getOutOfLineStorage();
----------------
nit: again drop `this`


================
Comment at: llvm/include/llvm/ADT/FunctionExtras.h:190
 
   template <typename CallableT>
   static ReturnT CallImpl(void *CallableAddr, AdjustedParamT<ParamTs>... Params) {
----------------
nit: rename this to `CalledAsT` ?


================
Comment at: llvm/include/llvm/ADT/FunctionExtras.h:211
+
+  // By default, we need to an object that contains all the different
+  // type erased behaviors needed. Create a static instance of the struct type
----------------
s/to an/an/


================
Comment at: llvm/include/llvm/ADT/FunctionExtras.h:214
+  // here and each instance will contain a pointer to it.
+  template <typename CallableT, typename CalledAs, typename Enable = void>
+  static NonTrivialCallbacks Callbacks;
----------------
move and destroy pointers don't really depend on `CallableT` maybe change this to only take `CalledAsT` ?


================
Comment at: llvm/include/llvm/ADT/FunctionExtras.h:221
+  static TrivialCallback
+      Callbacks<CallableT, CalledAs, EnableIfTrivial<CallableT>>;
+
----------------
again this also doesn't depend on `CallableT`


================
Comment at: llvm/include/llvm/ADT/FunctionExtras.h:224
+  // A simple tag type so the call-as type to be passed to the constructor.
+  template <typename T> struct CallAs {};
+
----------------
maybe `CalledAs` ?


================
Comment at: llvm/include/llvm/ADT/FunctionExtras.h:229
+  // (We always store a T, even if the call will use a pointer to const T).
+  template <typename CallableT, typename CallAsT>
+  UniqueFunctionBase(CallableT Callable, CallAs<CallAsT>) {
----------------
s/CallAsT/CalledAsT/

(or rename in the other way for the rest of the file)


================
Comment at: llvm/include/llvm/ADT/FunctionExtras.h:347
+  R operator()(P... Params) {
+    return this->getCallPtr()(this->getCalleePtr(), Params...);
   }
----------------
drop this ?


================
Comment at: llvm/include/llvm/ADT/FunctionExtras.h:358
+  unique_function() = default;
+  unique_function(std::nullptr_t /*null_callable*/) {}
+  unique_function(unique_function &&) = default;
----------------
call `Base(nullptr_t)`, also in the mutable version?


================
Comment at: llvm/include/llvm/ADT/FunctionExtras.h:370
+  R operator()(P... Params) const {
+    return this->getCallPtr()(this->getCalleePtr(), Params...);
   }
----------------
drop this ?


================
Comment at: llvm/unittests/ADT/FunctionExtrasTest.cpp:230
+  // Can assign from const lambda.
+  unique_function<int(int) const> Plus2 = [X(std::make_unique<int>(2))](int Y) {
+    return *X + Y;
----------------
only-moveable captures are already tested elsewhere. is there a specific reason for this capture ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82581/new/

https://reviews.llvm.org/D82581