Is this way acceptable for adding a unique_ptr to a vector?

Question

Say I have a Box. It contains Things, which are made by the Box, and only the Box. The Things are also uncopyable.

There is also an ownership relation, so I'd like to use unique_ptrs. (If the Box goes, everything goes with it.)

#include <memory>
#include <vector>

class Thing {
    friend class Box;
    public:
    ~Thing() {/* delete thing */};

    private:
    Thing() {/* construct thing */};
    Thing(const Thing& other) = delete;
    Thing& operator=(Thing&& other) = delete;
};

class Box {
    private:
    std::vector<std::unique_ptr<Thing> > contents{};

    public:
    void makeThing() {
        contents.push_back(nullptr);
        contents.back().reset(new Thing()); // I can live with the 'new' keyword here.
    }
};

My question is: is it bad to add to the vector by pushing a nullptr then resetting it to a new object? (Aside from the new keyword, which seems fine if the constructor doesn't throw?)

Note: I have already seen alternative ways to overcome the private constructor and deleted copy semantics. But is there anything dangerously wrong with what I wrote above?

Note 2: Explicitly stating it here since it already caught a few people: std::make_unique will not work as the Thing constructor is private.

Answer 1

Normally you would use make_unique. But since the constructor of Thing is private and Box is friend, you need to construct the new Thing in Box, you can't use std::make_unique. But your approach is ... weird.

If you can't use make_unique it is very easy to create the possibility of leaks. For instance, the first obvious thing to do:

contents.emplace_back(new Thing()); // this can leak

Will leak (if the vector resizing fails)

There are solutions to solve this, like the longer:

contents.emplace_back(std::unique_ptr<Thing>(new Thing()));

Or reserving ahead:

contents.reserve(contents.size() + 1); // this prevents the amortized constant complexity
                                       // of `push_back`
                                       // resulting in worst performance
contents.emplace_back(new Thing);

But the problem still remains: It's easy to write a leaky code and the correct code does extra things which can look unnecessary so you need to comment and document their purpose so that the next programmer doesn't optimize them.

The best solution in my opinion is to make the constructor available to make_unique:

class Thing {

    friend std::unique_ptr<Thing> std::make_unique<Thing>();
    // ...
};

class Box {
    // ...

    void makeThing() {
        contents.emplace_back(std::make_unique<Thing>());
    }
};

Since making make_unique friend defeats the purpose of private constructor, I like more n314159's solution of using a private allocate_thing method.

---

Thanks to all of the commenters down bellow for helping make this post correct and better.

Answer 2

I would go with this construct:

auto thing = std::unique_ptr<Thing>(new Thing());
contents.push_back(std::move(thing));

Because this describes more what you are actually want to do. Your Box object creates a Thing object, passes the ownership to a unique_ptr and then you move that unique_ptr to contents.

And don't use contents.emplace_back(new Thing()); one reason for sure it that it might leak. But the - imho - even more important part is readability and maintainability, and if you write contents.emplace_back(new Thing()); then you need to check if contents.emplace_back really claims ownership (that contents is really of the type std::vector<std::unique_ptr<Thing> > and not std::vector<Thing *>, with std::move(thing) and thing being a unique_ptr it is clear that the ownership is moved.

The downside with your solution is that at the time of reading contents.push_back(nullptr); it is not clear why nullptr is added and you need to read the next line to understand why.

Answer 3

I've been playing around with this and just noticed that I landed approximately where @n31459 ended up. The only addition is that it uses perfect forwarding to the Thing constructors so you only need one makeThing function and factory function in Thing if you add more constructors later.

#include <memory>
#include <utility>
#include <vector>

class Thing {
public:
    ~Thing() = default;

private:
    explicit Thing(int Value) : value(Value) {}

    Thing(const Thing&) = delete;
    Thing(Thing&&) noexcept = delete;
    Thing& operator=(const Thing&) = delete;
    Thing& operator=(Thing&&) noexcept = delete;

    int value; // some data

    template<class... Args>
    static std::unique_ptr<Thing> make_unique(Args&&... args) {
        return std::unique_ptr<Thing>{new Thing(std::forward<Args>(args)...)};
    }

    friend class Box;
};

class Box {
public:
    template<class... Args>
    void makeThing(Args&&... args) {
        contents.emplace_back(Thing::make_unique(std::forward<Args>(args)...));
    }

private:
    std::vector<std::unique_ptr<Thing>> contents{};
};

Answer 4

I think @t.niese has the way to do it. I want to add an alternative that hides the new a bit and maybe prevents anyone from refactoring with emplace_back(new Thing) (that can leak).

#include <memory>
#include <vector>

class Thing {
    friend class Box;
    public:
    ~Thing() {/* delete thing */};

    private:
    Thing() {/* construct thing */};
    Thing(const Thing& other) = delete;
    Thing& operator=(Thing&& other) = delete;

    static std::unique_ptr<Thing> allocate_thing() {
        return std::unique_ptr<Thing>(new Thing);
    }
};

class Box {
    private:
    std::vector<std::unique_ptr<Thing> > contents{};

    public:
    void makeThing() {
        // DO NOT replace with emplace_back. Temporary prevents leekage in case of exception.
        contents.push_back(Thing::allocate_thing());
    }
};

What I don't like about this, is that one could imagine that Thing::allocate_thing() does some weird magic (which is not the case) and hence this makes the code more confusing.

I further want to say that maybe you should think about your design. While trying to reduce the access to Thing is good, maybe there is a better way to do that (maybe a anonymous namespace? Of course this will not prevent someone from creating an object if they can do a decltype of an element of the vector at some point). Maybe you can get some good recommnendations if you present your problem in more generality and not just ask for comments on this one solution.