3

I was updating some old code that uses auto_ptr to use unique_ptr instead. It was mostly a search and replace job, but I found I was getting a compilation error in one place where the code returns a unique_ptr.

Here is an example that illustrates the issue:

#include <string>
#include <iostream>
#include <memory>
#include <utility>

using namespace std;

struct Foo {
    unique_ptr<string> us;

    Foo() {
        this->us = unique_ptr<string>(new string("hello"));
    }

    unique_ptr<string> bar() {
        return this->us;
    }

};


int main(int argc, const char **argv) {

    Foo foo;
    unique_ptr<string> s = foo.bar();
    cout << *s << endl;

}

When I compile that, I get this:

t1.cpp: In member function ‘std::unique_ptr<std::basic_string<char> > Foo::bar()’:
t1.cpp:17:16: error: use of deleted function ‘std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = std::basic_string<char>; _Dp = std::default_delete<std::basic_string<char> >]’
   return this->us;
                ^~
In file included from /opt/rh/devtoolset-7/root/usr/include/c++/7/memory:80:0,
                 from t1.cpp:4:
/opt/rh/devtoolset-7/root/usr/include/c++/7/bits/unique_ptr.h:388:7: note: declared here
       unique_ptr(const unique_ptr&) = delete;
       ^~~~~~~~~~

If I alter the line with the error to specify a move like this:

        return move(this->us);

then it works.

I have found multiple references indicating that a move should not be required - for example this SO question, and these guidelines from the chromium project.

My question is: why does the move need to be explicitly specified in this case? Is it related to fact that I am returning the value of an instance variable somehow?

Apologies in advance if this is a dupe - I feel sure it would have been asked before, but I struggle to come up with search terms that find it.

StoryTeller - Unslander Monica
  • 148,497
  • 21
  • 320
  • 399
harmic
  • 22,855
  • 3
  • 52
  • 72
  • If the compiler were to move that out on you without telling you, it could unexpectedly break your class invariant. – chris Apr 05 '19 at 06:45
  • This is not the right dupe. OP wants to use the pointer in two different places at the same time. SO it's not about returning the unique_ptr, but using shared_ptr instead ! – Christophe Apr 05 '19 at 06:53
  • @Christophe: The linked post recommends using `shared_ptr` – P.W Apr 05 '19 at 07:01

5 Answers5

6

It's all about semantics. A unique_ptr is a unique ownership, so it would be either returned or in us, but not both. The rules of the standard fortunately protect you against this misunderstanding.

What you can do is return a temporary unique_ptr (which will be moved):

unique_ptr<string> bar() {
    return make_unique<string>("hello, world");
}

But if you need a copy of the unique_ptr in several places, which seems to be the case according to your main(), you are not looking for a unique but a shared ownership. So you should go to a shared_ptr:

struct Foo {
    shared_ptr<string> us;

    Foo() {
        us = make_shared<string>("hello, world");
    }
    shared_ptr<string> bar() {
        return us;
    }
};

int main(int argc, const char **argv) {
    Foo foo;
    auto s = foo.bar();
    cout << *s << endl;
}
Christophe
  • 54,708
  • 5
  • 52
  • 107
  • `std::shared_ptr` removes the need to think about ownership and lifetimes, and that, in turn, leads to poor designs in 99% of cases when it is unclear who owns what and objects cannot destroy because of cyclic references. I would recommend avoiding `std::shared_ptr` as much as possible. Also, `std::shared_ptr` is one of the worst performing pointers because its size is double of a plain pointer, plus the control block with two atomic counters, and it always uses atomic increments when copying and busy loops for decrements when destroying even when the pointers never cross threads. – Maxim Egorushkin Apr 05 '19 at 07:20
  • @MaximEgorushkin I fully agree with you on the need to carefully think about ownership before choosing to use a shared_ptr ! I would also pretend that if an owner acquires a unique_ptr it should not disclose it. So exposing it publicly, trying to return it, or trying to return the underlying pointer already suggests a weakness in the design. Nevertheless, Rome wasn’t built in one day and OP mentions a legacy code base to maintain. I think that moving to shared_ptr and unique_ptr will help OP to reflect better on ownership, and at a later stage refactor step by step to improve the design. – Christophe Apr 05 '19 at 07:30
5

The references you found all mention that the object returned must be a function local variable. You return a member. Member variables will not be implicitly treated as rvalues when returned.

It's not without reason. A class usually doesn't want to lose ownership of its resources unless done explicitly.

A function local is about to die anyway when the function returns, so it's a worthwhile optimization to let the compiler implicitly cannibalize it.

StoryTeller - Unslander Monica
  • 148,497
  • 21
  • 320
  • 399
3

You are trying to return Foo::us member variable of type std::unique_ptr<>. The sole purpose of std::unique_ptr<> is to be non-copyable but moveable. This is why that return requires std::move which resets Foo::us member variable to null.

bar function should probably return that std::string by value or a const reference:

string const& bar() const {
    return *this->us;
}

And then:

int main() {
    Foo foo;
    string const& s = foo.bar();
    cout << s << endl;
}
Maxim Egorushkin
  • 119,842
  • 14
  • 147
  • 239
2

Used correctly std::auto_ptr has the same purpose as std::unique_ptr, it holds a unique reference to a single object which is deleted on destruction of the pointer.

As std::auto_ptr predates move semantics it has a copy constructor which behaves like a move constructor. It is very easy to accidentally copy a std::auto_ptr and not realise your original std::auto_ptr is now empty.

As std::unqiue_ptr is only moveable and not copyable this situation is avoided. You have to explicitly give up your ownership of the pointer using std::move.

The exception to this is with local stack allocated std::unique_ptrs, these would be destroyed at the end of the current scope anyway so the language allows you to return a std::unique_ptr without using std::move. As your us variable is not a local variable you aren't allowed to return it directly to protect you from accidentally releasing ownership.

Alan Birtles
  • 22,711
  • 4
  • 22
  • 44
  • This is a nice explanation about why auto_ptr was more permissive than it should in view of the unchanged semantics ! Definitively something to know for all those who are maintaining older code bases. – Christophe Apr 05 '19 at 07:36
1

This shows the exact danger of auto_ptr and why it was removed.

If you imagine calling the function twice you can see what the issue is.

auto_ptr<string> Foo::bar() {
    return this->us;
}

int func() {
  Foo foo;
  auto_ptr<string> a = foo.bar();
  auto_ptr<string> b = foo.bar();//uh oh, null ptr
}

If you like this behavior, you can emulate it very easily with unique_ptr

#include <string>
#include <iostream>
#include <memory>
#include <utility>

using namespace std;

struct Foo {
    unique_ptr<string> us;

    Foo() {
        this->us = unique_ptr<string>(new string("hello"));
    }

    unique_ptr<string> bar() {
        unique_ptr<string> local_us(us.release());
        return local_us;
    }

};


int main(int argc, const char **argv) {

    Foo foo;
    unique_ptr<string> a = foo.bar();
    unique_ptr<string> b = foo.bar();//b is a nullptr
    cout << *s << endl;

}
PeterT
  • 6,683
  • 1
  • 21
  • 27
  • `unique_ptr local_us(us.release()); return local_us;` - that's what `return move(us);` does but with style. – Maxim Egorushkin Apr 05 '19 at 07:59
  • @MaximEgorushkin I was under the impression that moving from the same object twice is not a good idea and "valid but unspecified state" seems not the best state to leave an object in. – PeterT Apr 05 '19 at 08:02
  • 2
    @PeterT in general the moved from object is in an unspecified state but specific classes can define what that state is, in the case of `unique_ptr` that state is a null pointer – Alan Birtles Apr 05 '19 at 08:56
  • @PeterT As Alan mentioned, a moved from `unique_ptr` is `null`. Not because the standard requires that (i don't think it does) but because any value other than `null` would be wrong. Generally, a moved from state is equivalent to a default constructed object state, unless the class doesn't have a default constructor. It is useful to think about `move` as `swap` but with possibly immediate destruction of the `rhs`. – Maxim Egorushkin Apr 05 '19 at 09:11
  • @AlanBirtles true, I didn't know that the standard explicitly stated that in 23.11.1.4.2 . Since I'm mostly lazy and look at cppreference – PeterT Apr 05 '19 at 09:12
  • 1
    @MaximEgorushkin the standard does define it to be null, but if it didn't a newly constructed object would also be correct – PeterT Apr 05 '19 at 09:15