10

I wonder if there are any legitimate reasons to return unique pointers by reference in C++, i.e. std::unique_ptr<T>&?

I've never actually seen that trick before, but the new project I've got seems to use this pattern a lot. From the first glance, it just effectively breaks / circumvents the "unique ownership" contract, making it impossible to catch the error in compile-time. Consider the following example:

class TmContainer {
public:
    TmContainer() {
        // Create some sort of complex object on heap and store unique_ptr to it
        m_time = std::unique_ptr<tm>(new tm());
        // Store something meaningful in its fields
        m_time->tm_year = 42;
    }

    std::unique_ptr<tm>& time() { return m_time; }

private:
    std::unique_ptr<tm> m_time;
};

auto one = new TmContainer();
auto& myTime = one->time();
std::cout << myTime->tm_year; // works, outputs 42
delete one;
std::cout << myTime->tm_year; // obviously fails at runtime, as `one` is deleted

Note that if we'd returned just std::unique_ptr<tm> (not a reference), it would raise a clear compile-time error, or would force use to use move semantics:

// Compile-time error
std::unique_ptr<tm> time() { return m_time; }

// Works as expected, but m_time is no longer owned by the container
std::unique_ptr<tm> time() { return std::move(m_time); }

I suspect that a general rule of thumb is that all such cases warrant use of std::shared_ptr. Am I right?

Enlico
  • 23,259
  • 6
  • 48
  • 102
GreyCat
  • 16,622
  • 18
  • 74
  • 112
  • 5
    It doesn't really makes sense to return a reference to an owning `std::unique_ptr`, imho. – Ron Mar 05 '18 at 17:42
  • 3
    You'd return a reference to a `unique_ptr` for the same reason you'd return a reference to any other type of object. Because you want to give the caller the ability to manipulate it. – Benjamin Lindley Mar 05 '18 at 17:44
  • @BenjaminLindley Of course, formally it is like that. But could you provide any examples when returning reference to `unique_ptr` is not a hack, but legitimately the best / only solution? – GreyCat Mar 05 '18 at 17:52
  • Maybe, if I thought about it for a while. But even if I couldn't think of one, does not mean that there are none. – Benjamin Lindley Mar 05 '18 at 17:58
  • @BenjaminLindley would there be any advantage of using reference to `unique_ptr` rather than `shared_ptr`? – Killzone Kid Mar 05 '18 at 18:03
  • 2
    @KillzoneKid: Yes. With a `shared_ptr` (returned by value), you couldn't reset the original owner's pointer. – Benjamin Lindley Mar 05 '18 at 18:05
  • 2
    I don't accept this code in code review. Just return the raw pointer stored in it. – JVApen Mar 05 '18 at 18:46
  • 3
    This smells of refactoring gone bad. As if someone were tasked to replace all raw pointers in a codebase, and favor `unique_ptr`. – Drew Dormann Mar 05 '18 at 18:54
  • For your new project, I think you'll have to ask the other developers why they are using that pattern. Seems strange to me. I've never found need to use a return-by-reference for a unique_ptr or a shared_ptr. Perhaps they are conflating ownership with access...? – Eljay Mar 06 '18 at 11:24
  • 1
    @Ron Vector (or other container) of unique owners doesn't make sense? – curiousguy Mar 07 '18 at 03:27

3 Answers3

6

There are two use cases for this and in my opinion it is indicative of bad design. Having a non-const reference means that you can steal the resource or replace it without having to offer separate methods.

// Just create a handle to the managed object
auto& tm_ptr = tm_container.time();
do_something_to_tm(*tm_ptr);

// Steal the resource
std::unique_ptr<TmContainer> other_tm_ptr = std::move(tm_ptr);

// Replace the managed object with another one
tm_ptr = std::make_unique<TmContainer>;

I strongly advocate against these practices because they are error prone and less readable. It's best to offer an interface such as the following, provided you actually need this functionality.

tm& time() { return *m_time; }

std::unique_ptr<tm> release_time() { return {std::move(m_time)}; }

// If tm is cheap to move
void set_time(tm t) { m_time = make_unique<tm>(std::move(t)); }

// If tm is not cheap to move or not moveable at all
void set_time(std::unique_ptr t_ptr) { m_time = std::move(t_ptr); }
patatahooligan
  • 3,111
  • 1
  • 18
  • 27
  • "Steal the resource" implicitly replaces the resource with a null resource, and "Replace the resource" implicitly steals it (then immediately hands it off to `delete`). These are the same use-case (but your point about it being bad is spot on) – Caleth Mar 06 '18 at 12:11
  • I disagree on them being the same use case. One keeps the resource alive also leaves the container empty (which might not even be a valid state in some projects), while the other discards the resource and makes the container point to something valid. The underlying mechanics are similar, but they are logically distinct actions. Notice how in my preferred interface one is `release_time` and the other `set_time` and how interfaces might offer one of them but not the other if it doesn't make sense to use it. – patatahooligan Mar 06 '18 at 12:24
  • I don't think it's bad design, if used wisely, it's pretty powerful. Think of any STL container storing unique_ptr, you have a reference whenever you access them. – Masadow Jan 09 '19 at 14:08
  • In fact returning reference will cause issue if the `unique_ptr` contains `null_ptr`. – KRoy Oct 29 '19 at 23:39
1

This was too long of a comment. I don't have a good idea for requested use case. The only thing I imagine is some middle ware for a utility library.

The question is what do you want and need to model. What are the semantics. Returning reference does not anything useful that I know.

The only advantage in your example is no explicit destructor. If you wanted to make exact mirror of this code it'd be a raw pointer IMHO. What exact type one should use depends on the exact semantics that they want to model.

Perhaps the intention behind the code you show is to return a non-owning pointer, which idiomatically (as I have learned) is modeled through a raw pointer. If you need to guarantee the object is alive, then you should use shared_ptr but bear it mind, that it means sharing ownership - i.e. detaching it's lifetime from the TmContianer.

Using raw pointer would make your code fail the same, but one could argue, that there is no reason for explicit delete as well, and lifetime of the object can be properly managed through scopes.

This is of course debatable, as semantics and meaning of words and phrases is, but my experience says, that's how c++ people write, talk and understand the pointers.

luk32
  • 15,812
  • 38
  • 62
0

std::unique_ptr does not satisfy the requirements of CopyConstructible or CopyAssignable as per design.

So the object has to be returned as a reference, if needed.

Pavan Chandaka
  • 11,671
  • 5
  • 26
  • 34
  • 2
    The question is, when it's needed. Also it doesn't have to be returned as reference. A classic counter-example is returning a temporary `unique_ptr` by value, or through `std::move` which means giving up ownership. I think any `make_resource` implementation using smart pointers works this way. – luk32 Mar 05 '18 at 18:16
  • @luk32 yes I do agree. But I feel like, it is more than need, the design forced the individual to use return by reference. And I have a doubt in your comment, Is it possible to return a temporary unique_ptr by value? return by value requires a copy operation rt. – Pavan Chandaka Mar 05 '18 at 18:31
  • Since c++17 if the temporary is unnamed there is a guaranteed copy-elision, and before it's implicitly moved. See [how it works](https://ideone.com/5Us3Mh), and it even compiles for [c++11](https://godbolt.org/g/RPhNYJ). A resource is created within a method and it's ownership is passed, I think you can do it with explicit `std::move` and have class which gives back `unique_ptr` by value. This would mean that whatever you get was in possession of the class, and now it's the callers. This way an object can give out it's resources. – luk32 Mar 06 '18 at 10:35