array move operator= is actually a copy

[kfsone]

reproc/reproc++/include/reproc++/detail/array.hpp

Lines 23 to 30 in 08675b1

	array &operator=(array &&other) noexcept
	{
	if (&other != this) {
	data_ = other.data_;
	owned_ = other.owned_;
	other.data_ = nullptr;
	other.owned_ = false;
	}

The move-assignment operator is not actually moving but copying.

#include <iostream>
struct S {
  S(char *s) : s_(s) {}
  S(S&& rhs) : s_(rhs.s_) { rhs.s_ = nullptr; }
  S& operator=(S&& rhs) { s_ = rhs.s_; return *this; }
  ~S() { *s_ = 0; s_ = nullptr; }
  char *s_;
};

int main() {
  char word1[] = { "hello" };
  char word2[] = { "hello" };

  S s1(word1);
  std::cout << "s1.s_ = '" << s1.s_ << "'\n";
  s1 = std::move(S(word2));
  std::cout << "s1.s_ = '" << s1.s_ << "'\n";
}

https://gcc.godbolt.org/z/az44M1577

Program stdout
s1.s_ = 'hello'
s1.s_ = ''

Move should be implemented as swap or exchange:

  S(S&& rhs) : 
    s_(std::exchange(rhs.s_, nullptr))
    //^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^//
    {}
  S& operator=(S&& rhs) {
    std::swap(s_, rhs.s_);
    //^^^^^^^^^^^^^^^^^^//
    return *this;
  }

https://gcc.godbolt.org/z/5fzrhaGvd

  array(array &&other) noexcept 
    // if other == this, somehow, we'll take data_, replace it with nullptr,
    // and then replace that with the save of data_, etc.
    : data_(std::exchange(other.data_, nullptr))
    , owned_(std::exchange(other.owned_, false))
  {
  }

  array &operator=(array &&other) noexcept
  {
    // transfer whatever we owned previously to the rvalue, so it can clean
    // up any data we previously had.
    //   {
    //     array one(...);
    //     array two(...);
    //     ...
    //     two = std::move(one);  // two's data transferred to one, which the
    //                            // compiler may now destruct here or later.
    //     ...
    //     one = array(...);      // what two allocated is moved to the temporary,
    //                            // and then destructed here in rvalue::~array()
    //     ...
    //   } // << whatever is still in two destructed here
    // this also eliminates the need for the potentially branch-inducing self check.
    std::swap(data_, other.data_);
    std::swap(owned_, other.owned_);

    return *this;
  }

[kfsone]

The std::exchange requires c++14, however, so you may want to just use

: data_(other.data_), owned_(other.owned_) { other.data_ = nullptr; other.owned_ = false; }

[DaanDeMeyer]

@kfsone Sure, it's not a pure move but we're copying a pointer which is instant? Why is this a problem?

[kfsone]

Exactly that you are copying the pointer. If the object you are copying from is a temporary, it will be destroyed immediately after this method. The pointer you copied is now invalid.

std::move and std::forward are just casts, it's the operators that have to actually perform the movement.

[kfsone]

Here's a demonstrative example, change the "MOVE_NOT_COPY" on the first line from 0 to 1 to see it operate without leaking/multi-deleting.

https://gcc.godbolt.org/z/PWxExbPx1

Note in particular where A#4 and A#7 are ~d relative to the end of scope.

Passing an rvalue reference to an object makes it an rvalue. The only reason that the language doesn't automatically delete named rvalues is so that you can reuse their storage, which in turn depends on them having good move operators.

{
  A a1{};
  A a2 = std::move(a1);   // a1 is now dead and the compiler COULD dtor it here, but that would add extra requirements on dtors.
  a1 = A{};  // create temporary, rvalue assign it to a1, dtor the temporary.
} // dtor a2, dtor a1