Function signature for potentially failed unique_ptr move?

user_1831108 0
Viewed 95

Say I am writing a enqueue() function that takes in a unique_ptr, but I only want to claim its ownership when enqueue returns success. If the queue is full I want to leave the unique_ptr intact (user can retry with the same item later)

bool enqueue(std::unique_ptr&& item){
  if(!vector.full()){
    vector.emplace(std::move(item));
    return true;
  }
  return false;
}
// usage
auto item_ptr = make_unique<>();
while(!enqueue(std::move(item_ptr))){
// item_ptr is not moved
}

I can also define the function to take a lvalue reference instead

bool enqueue(std::unique_ptr& item)
while(!enqueue(item_ptr)){
// item_ptr is not moved
}

I am not sure which one to pick, they all seemed a little anti-patterned since usually std::move indicates the deletion of a unique_ptr (most of the time, I work with function that takes unique_ptr by value), maybe there's a better solution?

move-semantics unique-ptr c++
4 Answers

The first version with the RValue reference accepts a temporary. Thus, if the move didn't happen you have an instance in a temporary std::unique_ptr which will be deleted soon. This is what makes me uncertain whether it's a good choice (or inviting for surprising effects).

At least, one should be aware of this.

Concerning the question

what happens if you do while (!enqueue(make_unique<>()))...

I made an MCVE (just to be sure):

#include <memory>
#include <iostream>

struct Test {
  Test() { std::cout << "Test::Test()\n"; }
  ~Test() { std::cout << "Test::~Test()\n"; }
};

bool enqueue(std::unique_ptr<Test>&& item)
{
  return false;
}

int main()
{
  for (int i = 0; i < 3 && !enqueue(std::make_unique<Test>()); ++i);
}

Output:

Test::Test()
Test::~Test()
Test::Test()
Test::~Test()
Test::Test()
Test::~Test()

Demo on coliru

If it's intended to prevent "abuse" and ensure that enqueue() isn't called for temporaries the RValue version could be dropped.

user_74785970

You could return a unique_ptr from your function. If it failed return the original. If it succeeded return an empty unique_ptr.

Then you could call it like:

#include <iostream>
#include <memory>
#include <vector>

int counter = 10;

template <typename T> std::unique_ptr<T> enqueue(std::unique_ptr<T> p) {
  if (--counter == 0)
    return std::unique_ptr<T>();
  return p;
}

int main() {
  auto item_ptr = std::make_unique<int>(8);
  while (item_ptr = enqueue(std::move(item_ptr))) {
    std::cout << "looping\n";
  }
  std::cout << "moved\n";
  return 0;
}

Actually, I'd better go try this out. Yeah my first idea had a bug. Ripping out that ! from the while loop. Always test. :-)

Zan_Lynx_134220

The frame of mind I like to have about std::move is that it's only me indicating I'm fine with the object being modified. It's not about causing deletion, it's about giving permission for potentially destructive stuff to happen.

Your first option seems OK to me. Callers have to explicitly give permission by marking their lvalues with std::move, and they can check if something indeed happened by inspecting the return value of the function. So they won't have objects they care about suddenly disappearing. It's good enough IMO.

But like another answer said, this version also gives the option of passing a temporary unique_ptr. If the pointless work in that case bothers you, then we should probably go with another approach.

One guideline I like to take from the Google Style Guide sometimes, is that [input/]output parameters are passed by non-owning raw pointer. This basically restricts them to being lvalues, while retaining the explicitness required in allowing for their modification. So you could do this:

bool enqueue(std::unique_ptr<T>* item){
  if(!vector.full()){
    vector.emplace(std::move(*item));
    return true;
  }
  return false;
}

// usage
auto item_ptr = make_unique<T>();
while(!enqueue(&item_ptr)){
// item_ptr is not modified
}

Under such a style guide, the address-of operator & serves the same purpose as std::move. It's us telling the function: "Go ahead, modify it if you need to."

user_8176430

This solution seems kind of tricky to me, as you're passing the unique_ptr, but it may or may not be claimed, so there's a need to somehow handle this kind of behavior: like, returning an empty unique_ptr in case of a success or an error/success code, doing redundant or hard-to-understand checks and so on. I think what you can do is check if your queue has free space externally, with the help of full() method or something like that. Then your code will look something like that:

if(!full()) {
    enqueue(std::move(...));
}
user_163317940
Related