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

[Sam McCall via Phabricator via llvm-commits]

sammccall marked 12 inline comments as done.
sammccall added inline comments.


================
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;
----------------
kadircet wrote:
> move and destroy pointers don't really depend on `CallableT` maybe change this to only take `CalledAsT` ?
I don't think using e.g. MoveAs<CalledAsT> will work: you almost always can't move from a `const T`.

You could do something like `MoveAs<remove_cv_t<CalledAsT>>` or have `MoveAs` const_cast. But that's just a different design, and I don't think particularly a better one: having both CallableT and CalledAsT explicitly specified (with the base class responsible for ensuring they're compatible) seems more robust than trying to make assumptions about how to calculate one from the other.

For example, when rvalue-ref qualified functions get added, I think it's nice not to have to worry about updating the metaprogramming to calculate the underlying type.


================
Comment at: llvm/include/llvm/ADT/FunctionExtras.h:221
+  static TrivialCallback
+      Callbacks<CallableT, CalledAs, EnableIfTrivial<CallableT>>;
+
----------------
kadircet wrote:
> again this also doesn't depend on `CallableT`
This is a specialization of the template above, so I don't think there's anything we can change here if the above stays the same.


================
Comment at: llvm/include/llvm/ADT/FunctionExtras.h:347
+  R operator()(P... Params) {
+    return this->getCallPtr()(this->getCalleePtr(), Params...);
   }
----------------
kadircet wrote:
> drop this ?
it's required here to call a method from a dependent base.
(That's where all the other extra `this`es came from - code that I had in one of these subclasses and then moved)


================
Comment at: llvm/include/llvm/ADT/FunctionExtras.h:358
+  unique_function() = default;
+  unique_function(std::nullptr_t /*null_callable*/) {}
+  unique_function(unique_function &&) = default;
----------------
kadircet wrote:
> call `Base(nullptr_t)`, also in the mutable version?
This is "just" syntax sugar for the default constructor, and that's hardly obscure.

I think it's unneccesarily confusing to implement the public API in the base class where it doesn't save any complexity, so I've removed the nullptr_t from the base instead.

(I'm torn on `operator bool` as it's the last thing publicly exposed from the base, but I don't think it matters much)


================
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;
----------------
kadircet wrote:
> only-moveable captures are already tested elsewhere. is there a specific reason for this capture ?
Have to make the machinery sweat a little!

More seriously, if the underlying functor is copyable, then MoveImpl<const lambda> *would* compile... but does a copy.

So there are reasonable-looking implementations where move works and const works but move const doesn't work. (but I admit I only worked this out after the fact).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82581