How to properly forward unique_ptr?

Question

What is generally the proper way to forward an std::unique_ptr?

The following code uses std::move, which I thought was the considered practice, but it crashes with clang.

class C {
   int x;
}

unique_ptr<C> transform1(unique_ptr<C> p) {
    return transform2(move(p), p->x); // <--- Oops! could access p after move construction takes place, compiler-dependant
}

unique_ptr<C> transform2(unique_ptr<C> p, int val) {
    p->x *= val;
    return p;
}

Is there a more robust convention than simply making sure you get everything you need from p before transferring ownership to the next function via std::move? It seems to me using move on an object and accessing it to provide a parameter to the same function call could be a common mistake to make.

Answer 1

The code is not fine.

• std::move is nothing but a cast (g++: something like static_cast<typename std::remove_reference<T>::type&&>(value))
• Value computation and side effects of each argument expression are sequenced before execution of the called function.
• However, the initialization of function parameters takes place in the context of the calling function. (Thanks to T.C)

Quotes from the draft N4296:

1.9/15 Program execution

[...] When calling a function (whether or not the function is inline), every value computation and side effect associated with any argument expression, or with the postfix expression designating the called function, is sequenced before execution of every expression or statement in the body of the called function. [...]

5.2.2/4 Function call

When a function is called, each parameter (8.3.5) shall be initialized (8.5, 12.8, 12.1) with its corresponding argument. [ Note: Such initializations are indeterminately sequenced with respect to each other (1.9) end note ] [...] The initialization and destruction of each parameter occurs within the context of the calling function. [...]

A sample (g++ 4.8.4):

#include <iostream>
#include <memory>
struct X
{
    int x = 1;
    X() {}
    X(const X&) = delete;
    X(X&&) {}
    X& operator = (const X&) = delete;
    X& operator = (X&&) = delete;
};


void f(std::shared_ptr<X> a, X* ap, X* bp, std::shared_ptr<X> b){
    std::cout << a->x << ", " << ap << ", " << bp << ", " << b->x << '\n';
}

int main()
{
    auto a = std::make_unique<X>();
    auto b = std::make_unique<X>();
    f(std::move(a), a.get(), b.get(), std::move(b));
}

The output may be 1, 0xb0f010, 0, 2, showing a (zero) pointer moved away.

Answer 2

As noted in Dieter Lücking's answer, the value computations are sequenced before function body, so the std::move and operator -> are sequenced before the body of the function --- 1.9/15.

However, this does not specify that the parameter initialization is done after all of those computations, they can appear anywhere with regard to each other, and to non-dependent value computations, as long as they are done before the function body --- 5.2.2/4.

This means the behaviour is undefined here, as one expression modifies p (moving into a temporary argument) and the other uses the value of p, see https://stackoverflow.com/a/26911480/166389. Although as mentioned there, P0145 proposes to fix the evaluation order to left-to-right (in this case). Which would mean your code is broken, but transform2(p->x, move(p)) would do what you want. (Corrected, thanks to T.C.)

As far as idioms go to avoid this, consider David Haim's approach taking the unique_ptr<C> by reference, although that's pretty opaque to the caller. You're signalling something like "May modify this pointer". unique_ptr's moved-from state is reasonably clear, so this isn't likely to bite you as badly as if you move from a passed-in object reference or something.

In the end, you need a sequence point between using p and modifying p.

Answer 3

Generally speaking, no, there isn't a fool-proof method of doing this without taking care of the small details. Where it gets really sneaky is in member-initialization-lists.

Just an as example that goes one step beyond yours, what happens if p->x is itself an object whose lifetime depends on *p, and then transform2(), which is effectively unaware of the temporal relationship between its arguments, passes val onwards to some sink function, without taking care to keep *p alive. And, given its own scope, how would it know it should?

Move semantics are just one of a set of features that can be easily abused, and needs to be handled with care. Then again, that's part of the essential charm of C++: it requires attention to details. In return for that added responsibility, it gives you more control—or is it the other way around?

Answer 4

Stop doing more than one thing on a single line, unless the operations are non-mutating.

Code isn't faster if it is all on one line, and it is often less correct.

std::move is a mutating operation (or more accurately, it flags an operation to follow as "mutate ok"). It should be on its own line, or at the least it should be on a line with no other interaction with its parameter.

This is like foo( i++, i ). You modified something and also used it.

If you want a universal brainless habit, simply bind all arguments in std::forward_as_tuple, and call std::apply to call the function.

unique_ptr<C> transform1(unique_ptr<C> p) {
  return std::experimental::apply( transform2, std::forward_as_tuple( std::move(p), p->x ) );
}

which avoids the problem because the mutation of p is done on a line different than the reading of the p.get() address in p->x.

Or, roll your own:

template<class F, class...Args>
auto call( F&& f, Args&&...args )
->std::result_of_t<F(Args...)>
{
  return std::forward<F>(f)(std::forward_as_tuple(std::forward<Args>)...);
}
unique_ptr<C> transform1(unique_ptr<C> p) {
  return call( transform2, std::move(p), p->x );
}

The goal here is to order the evaluation-of-parameter expressions separately from the evaluation-of-parameter-initialization. It still doesn't fix some move-based issues (like SSO std::basic_string move-guts and reference-to-char issues).

Next to that, hope that a compiler will add warnings for unordered mutate-and-read in the general case.

Answer 5

Since you do not really need to access p after it was moved, one way would be to get p->x before the move and then use it.

Example:

unique_ptr<C> transform1(unique_ptr<C> p) {
    int x = p->x;
    return transform2(move(p), x);
}

Answer 6

ammm... not much of an answer but a suggestion - why pass the ownership of the unique_ptr in the first place? it seems that transformXXX play with the integer value, why does memory management has to play a part here?

pass the unique_ptr by reference :

unique_ptr<C>& transform1(unique_ptr<C>& p) {
    return transform2(p, p->x); // 
}

unique_ptr<C>& transform2(unique_ptr<C> p&, int val) {
    p->x *= val;
    return p;
}

pass ownership outside these functions. create specific functions which this is their job. seperate algebric logic from memory management.