CODEWITHSUNDEEP
c++move-semanticsunique-ptr
James Magnus
James Magnus

Reputation: 329

std::unique_ptr test before taking ownership

I have a class which is basically a queue used to transfer dynamically allocated objects between 2 threads. The first thread creates the objects and the second one consumes them. I use std::unique_ptr to pass the objects ownership from thread 1 to thread 2.

Actually calls to the method that put the objects into the queue is like this:

queue.put(std::move(unique_ptr_to_my_object));

and the signature:

bool Queue::put(std::unique_ptr<T> p);

The problem is the put() method has to check some condition to decide if the object could be added to the queue. In case the condition is false the method simply returns false to indicate it can't add the object to the queue, but the object is destroyed because ownership has already be taken by put().

So I wanted to know if it's ok to rewrite put() like this or if there is a better solution:

bool Queue::put(std::unique_ptr<T> &ref) {
    if(CANNOT_ADD)
        return false; // ownership remains in the calling function
    std::unique_ptr<T> p = std::move(ref); // we know we can add so take ownership
    /* ... */
}

Upvotes: 4

Views: 287

Answers (2)

Slava
Slava

Reputation: 44238

You can change signature of the function to:

std::unique_ptr<T> Queue::put(std::unique_ptr<T> p);

so if that function cannot take that object it would return that pointer back, or nullptr otherwise. Another solution is to take ownership conditionally:

bool Queue::put(std::unique_ptr<T> &&p);

and move out object only in success. Accepting rvalue reference vs lvalue one is better in this case due to at least 2 reasons:

  1. You can still pass temporary.
  2. You need to use std::move explicitly in calling code.

Though the fact that you can use that pointer after std::move make this variant is less readable.

Upvotes: 3

Martin Bonner supports Monica
Martin Bonner supports Monica

Reputation: 28987

Yes, that's fine. The alternative would be:

std::unique_ptr<T> Queue::put(std::unique_ptr<T> p) {
    if (CANNOT_ADD)
        return p; // Ownership returned to caller.
    /* ... */
    return {}; // Return empty nullptr to indicate success.
}

Your way has the advantage that the caller retains ownership if the code in ... throws.

Upvotes: 5

Related Questions