CODEWITHSUNDEEP
c++unique-ptr
Troganda
Troganda

Reputation: 131

move class instance holding a unique_ptr

I have a vector of class instances. Each of those instances has a unique_ptr to another class. Since I never try to copy the class instance or even share the pointer, I felt like unique_ptr are more appropriate than shared_ptrs since the pointer is not shared, but only accessible through the class instance.

Is it bad practice? And why wouldn't this work? I understand that copying an instance to a unique pointer would be ill-formed, but since I move it, I do not understand why this would not be allowed?

Would I have to create a custom move constructor? And what should it do?

The unique ptr should be deleted as soon as the object instance is being removed from the vector as there are no references left, right?

Code Example for better understanding:

class A {
private:
    int number;

public:
    void f() {
        std::cout << "The number is: " << number;
    }
    A(int i) : number{i} {}
    ~A() = default;
};

class B {
    std::unique_ptr<A> good_a;

    B() : good_a{ std::make_unique<A>(1) } {}
    ~B() = default;
};

int main()
{
    std::vector<B> all;

    B my_b(123);

    all.emplace_back(std::move(my_b));

}

Upvotes: 0

Views: 1755

Answers (3)

Marek R
Marek R

Reputation: 37647

Please read this answear.

Depending what is explicitly declared respective constructors with default implementation are implicitly defined or dropped. Rules are described by this table:

enter image description here

Since you have used explicit defined destructor (as default) you have disabled ("not declared") move constructor.

So to fix it you have to explicitly define move constructor or remove definition of destructor: https://godbolt.org/z/dr8KrsTfq

Upvotes: 1

eerorika
eerorika

Reputation: 238311

And why wouldn't this work?

What you described could work.

I do not understand why this would not be allowed?

What you described would be allowed.

Would I have to create a custom move constructor?

No, that wouldn't be necessary, unless you define other special member functions, or have other members (beside the unique pointer) that have deleted or private move constructor.

The unique ptr should be deleted as soon as the object instance is being removed from the vector as there are no references left, right?

Members are destroyed when the super object is destroyed. And the destructor of the unique pointer invokes the deleter on its owned pointer.

Whether there are references to the pointed object has no effect on whether it is deleted or not. Anything referring to the deleted object will be left dangling.

Is it bad practice?

There isn't necessarily anything particularly bad about what you described in general, but that depends on exact details.

One potential issue is that dynamic allocation can be expensive in some cases, and using it unnecessarily would then be unnecessarily expensive. As such, you should to have some reason to allocate the pointed objects dynamically rather than storing them directly as members.

Bugs in your example:

  • You attempt to initialise B(123) but B has no constructor accepting an integer.
  • You attempt to initialise a B outside a member function of B, but its constructors and the destructor have private access.
  • You have user declared destructor for B, but no user declared move constructor or assignment operators and therefore the class isn't movable, which is a requirement for storing in std::vector.

Here is a fixed version that doesn't use unnecessary dynamic allocation:

struct A {
    int number;
};

struct B {
    A good_a;
};

B my_b{123};
all.push_back(my_b);

Upvotes: 1

SergeyA
SergeyA

Reputation: 62563

This answer focuses on compilation error you seem to be having. Bad or good practice is left for others to chime in.

Your code have several errors there, but the main one seems to be that your custom B( ) constructor inhibits default move constructor. If you add it, your code becomes well-formed.

Here is a full working code for the reference:

#include <memory>
#include <vector>

class A {
private:
    int number;

public:
    void f();
    A(int i) : number{i} {}
    ~A() = default;
};

struct B {
    std::unique_ptr<A> good_a;

    B(int k) : good_a{ std::make_unique<A>(k) } {}

    B(B&& ) = default;
    B& operator=(B&& ) = default; // not needed for the example, but good for completeness
};

int main()
{
    std::vector<B> all;
    B my_b(123);
    all.emplace_back(std::move(my_b));
}

Upvotes: 5

Related Questions