Return a unique_ptr by reference

Question

So I've solved this problem, but I need your opinion if what I did is best practice.

A simple class holds a vector of unique_ptrs to order objects. I will explain the member variable null_unique below.

class order_collection {
    typedef std::unique_ptr<order> ord_ptr;
    typedef std::vector<ord_ptr> ord_ptr_vec;
    ord_ptr_vec orders;
    ord_ptr null_unique;
public:
    ...
    const ord_ptr & find_order(std::string);
    ....

So I need the users of this class to get access to the order unique_ptr if found. However I'm not going to move the object out of the vector so I'm returning the unique_ptr as const ref. My implementation of the find_order method:

const order_collection::ord_ptr & order_collection::find_order(std::string id) {
    auto it = std::find_if(orders.begin(),orders.end(),
                           [&](const order_collection::ord_ptr & sptr) {
                               return sptr->getId() == id;
                           });

    if (it == orders.end())
        return null_unique; // can't return nullptr here
    return *it;
}

Since I'm returning by reference I can't return a nullptr. If I try to do so, I get warning : returning reference to a temporary. And if nothing is found the program crashes. So I added a unique_ptr<order> member variable called null_unique and I return it when find doesn't find an order. This solves the problem and warning is gone and doesn't crash when no order is found.

However I'm doubting my solution as it make my class ugly. Is this the best practice for handling this situation?

You could always return a regular, non owning raw pointer. That would allow you to replace return null_unique; with return nullptr;. — NathanOliver
– NathanOliver, Commented Apr 13, 2017 at 20:08
NathanOliver beat me by a few seconds. To his point, do you have a reason why you want to return a const unique_ptr reference? You are not changing ownership, so you probably don't need to expose how these orders are memory-managed. — Drew Dormann
– Drew Dormann, Commented Apr 13, 2017 at 20:10
+1 for the raw pointer solution. I will also add that it's very common to return iterators to collections for find-like operations. You'd return the end iterator on not found. Perhaps with C++17 people will start moving towards a std::optional approach, though. — AndyG
– AndyG, Commented Apr 13, 2017 at 20:19

Vittorio Romeo · Accepted Answer · 2017-04-14 15:23:34Z

You should only return and accept smart pointers when you care about their ownership semantics. If you only care about what they're pointing to, you should instead return a reference or a raw pointer.

Since you're returning a dummy null_unique, it is clear that the caller of the method doesn't care about the ownership semantics. You can also have a null state: you should therefore return a raw pointer:

order* order_collection::find_order(std::string id) {
    auto it = std::find_if(orders.begin(),orders.end(),
                           [&](const order_collection::ord_ptr & sptr) {
                               return sptr->getId() == id;
                           });

    if (it == orders.end())
        return nullptr;
    return it->get();
}

Community · Accepted Answer · 2020-06-20 09:12:55Z

It doesn't really make sense to return a unique_ptr here, reference or otherwise. A unique_ptr implies ownership over the object, and those aren't really the semantics being conveyed by this code.

As suggested in the comments, simply returning a raw pointer is fine here, provided that your Project Design explicitly prohibits you or anyone on your team from calling delete or delete[] outside the context of the destructor of a Resource-owning object.

Alternatively, if you either have access to Boost or C++17, a std::optional<std::reference_wrapper<order>> might be the ideal solution.

std::optional<std::reference_wrapper<order>> order_collection::find_order(std::string id) {
    auto it = std::find_if(orders.begin(),orders.end(),
                       [&](const order_collection::ord_ptr & sptr) {
                           return sptr->getId() == id;
                       });

    if (it == orders.end())
        return {}; //empty optional object
    return **it; //will implicitly convert to the correct object type.
}

/*...*/

void func() {
    auto opt = collection.find_order("blah blah blah");
    if(!opt) return;
    order & ord = opt->get();
    /*Do whatever*/
}

(EDIT: In testing on the most recent version of MSVC 2017, it looks like std::reference_wrapper<T> will happily do an implicit conversion to T& if you tell it to. So replacing opt->get() with *opt should work exactly the same.)

As long as I'm here, I might point out that a std::vector<std::unique_ptr<type>> object has a very "Code Smell" sense to it. std::vector<type> implies ownership of the object as is, so unless you have a good reason to prefer this (maybe the objects are large, unmovable/uncopyable, and you need to insert and remove entries frequently? Maybe this is a polymorphic type?), you're probably better off reducing this to a simple std::vector.

EDIT:

The boost version is subtly different, because boost::optional has no restrictions against "optional references", which are specifically forbidden by the C++ Standard Library's version of std::optional. The boost version is actually going to be slightly simpler:

//return type changes, nothing else changes
boost::optional<order&> order_collection::find_order(std::string id) {
    auto it = std::find_if(orders.begin(),orders.end(),
                       [&](const order_collection::ord_ptr & sptr) {
                           return sptr->getId() == id;
                       });

    if (it == orders.end())
        return {}; //empty optional object
    return **it; //will implicitly convert to the correct object type.
}

/*...*/

//Instead of calling opt->get(), we use *opt instead.
void func() {
    auto opt = collection.find_order("blah blah blah");
    if(!opt) return;
    order & ord = *opt;
    /*Do whatever*/
}

Or if type is polymorphic, that's another reason to use std::vector<std::unique_ptr<type>> over std::vector<type>.
I get the following error with your solution: error: no viable conversion from returned value of type 'order' to function return type 'boost::optional<boost::reference_wrapper<order> >' return **it;
@bkarj In boost, you can just write boost::optional<order&>, you don't need to rely on a reference wrapper. The Standard Library chose to forbid "optional references" (here defined as an optional containing a reference) which is why we have to use a std::reference_wrapper to contain the reference (which is allowed). I'll update the code with what the boost version would look like.
very cool thank you. However it should be auto& in func() otherwise : error: call to implicitly-deleted copy constructor of 'std::__1::unique_ptr<order, std::__1::default_delete<order> >' auto opt = oc.find_order("blah blah blah");

Collectives™ on Stack Overflow

Return a unique_ptr by reference

2 Answers 2

Comments

EDIT:

8 Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

EDIT:

8 Comments

Related