[PATCH] D55045: Add a version of std::function that includes a few optimizations.

[Louis Dionne via Phabricator]

ldionne added inline comments.


================
Comment at: include/functional:2237
 {
-    *this = nullptr;
-    if (__f.__f_ == 0)
-        __f_ = 0;
-    else if ((void *)__f.__f_ == &__f.__buf_)
-    {
-        __f_ = __as_base(&__buf_);
-        __f.__f_->__clone(__f_);
-    }
-    else
-    {
-        __f_ = __f.__f_;
-        __f.__f_ = 0;
+    if (&__f != this) {
+        *this = nullptr;
----------------
jsoyke wrote:
> sbenza wrote:
> > ldionne wrote:
> > > sbenza wrote:
> > > > ldionne wrote:
> > > > > We didn't check for that before, did we? IOW the implementation worked transparently even in the case of a self move-assignment. Would it be possible to retain that property (and save this check in the current implementation)?
> > > > Without this check the code is equally correct, as in the moved-from value has a valid-but-unspecified state. The lhs happens to also be the moved-from value, but that is fine.
> > > Rephrasing to make sure I understand: If `this == &__f`, then after the assignment `__f` is in a valid-but-unspecified state (in this case it is empty). Since `this == &__f`, `*this` is also in the same valid-but-unspecified state.
> > > 
> > > I don't think this makes sense for self-assignment, because the `lhs` is usually understood to contain the value of the `rhs` after a move-assignment. IOW, the following should hold:
> > > 
> > > ```
> > > T x = ...;
> > > T copy = x;
> > > T y = std::move(x);
> > > assert(y == copy);
> > > ```
> > > 
> > > This is not the case here.
> > > 
> > The example above does not have any self-assignment (or any assignment).
> > 
> > My argument is that `y = std::move(y);` leaves `y` in an unspecified state. It is not required to be a noop. This is how it behaves today and it is up to spec.
> Self move assignment left the function empty before, preserving that behavior now.
Yeah, my example is obviously broken. What I meant is just that one would expect the `lhs` to contain the value of the `rhs`, not for that value to be nuked.

I understand the current implementation is valid in terms of the specification. I'm saying the specification does not satisfy the principle of least surprise. Leave it as-is.


================
Comment at: test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/alloc_F.pass.cpp:68
     std::function<FuncType> f2(std::allocator_arg, alloc, target);
-    assert(globalMemCounter.checkOutstandingNewEq(0));
+    assert(globalMemCounter.checkOutstandingNewEq(0) ||
+           globalMemCounter.checkOutstandingNewEq(1));
----------------
Should these checks be made to depend on `#ifdef _LIBCPP_ABI_OPTIMIZED_FUNCTION`?


================
Comment at: test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/alloc_rfunction.pass.cpp:92
         assert(f2.target<Ref>());
-        assert(f.target<Ref>()); // f is unchanged because the target is small
+        // assert(f.target<Ref>()); // f is unchanged because the target is small
     }
----------------
Those asserts should be removed too.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D55045