c++unique-ptrmove-semanticsownership-semantics

Extracting a std::unique_ptr member using std::move(), from a class that is invalid when null: how to avoid invisibly making object invalid?


Consider the following scenario. We have a class bar that owns a foo data object via a unique pointer. A bar must always be constructible into a valid state; that is, bar must always own some data, so data should never be null. We can call get_data() to peer into the data of bar, but the ownership of this data should not change for as long as bar is alive. So, data is added to bar when we construct it: the constructor takes in the unique pointer via move semantics.

Then we have baz, which can own multiple foo data. We can add each data one at a time by using add_data(), using std::move() to move the data into baz. So far, so good.

struct foo {};

class bar
{
public:
    bar(std::unique_ptr<foo> data) : data{ std::move(data) }
    {
        if (!this->data)
        {
            throw std::invalid_argument{ "Unexpected null pointer" };
        }
    }

    foo* get_data() { return (this->data).get(); }
private:
    std::unique_ptr<foo> data;
};

class baz
{
public:
    baz() = default;

    void add_data(std::unique_ptr<foo> data) { vec.push_back(std::move(data)); }
private:
    std::vector<std::unique_ptr<foo>> vec;
};

int main()
{
    // bar invalid_bar{ std::unique_ptr<foo>() }; // throws an exception

    std::unique_ptr<foo> data_0{ std::make_unique<foo>() };
    bar bar_0{ std::move(data_0) };

    std::unique_ptr<foo> data_1{ std::make_unique<foo>() };
    std::unique_ptr<foo> data_2{ std::make_unique<foo>() };
    std::unique_ptr<foo> data_3{ std::make_unique<foo>() };
    baz baz_0;
    baz_0.add_data(std::move(data_1));
    baz_0.add_data(std::move(data_2));
    baz_0.add_data(std::move(data_3));
}

Now I want to be able to merge the data in bar into that of baz:

void merge_data(baz& target, bar&& source)
{
    // ...
}

Contrary to the provided example code, these classes may instead represent large data structures, in which case unnecessary copying would best be avoid. The intention for this function is to take in a bar by move semantics, and whatever data was stored on it is now transferred to baz.

The problem is, this requires extracting the data from bar, along with its ownership. This means leaving a bar in an invalid null state. This extraction is fine if it happens inside merge_data(), since the bar is moved in, so the resulting invalid bar shouldn't see the light of day. However, extracting this data will involve adding a member function which is accesible to merge_data(). So, I consider adding the following public member to bar:

std::unique_ptr<foo> extract_data() { return std::move(this->data); }

This is ok, but there is one problem with this: since this is public, this means that there is a risk it could be called in the middle of the lifetime of a foo, risking the possibility of having foo in an invalid null state in the middle of use. I might be aware that calling this function will leave an invalid bar, and keep in mind to simply not use the bar after doing so. But I suspect it will be an unfortunate surprise to other uses who see the function and think that the extracted data is only being copied from bar!

How can I allow this extract_data() operation while avoiding leaving unsuspecting invalid bar objects?


Solution

  • According to your answer in the comments, whether moving bar leaves it in a valid state:

    I consider it here to be invalid; it doesn't uphold the invariant of the class, where it holds a non-null unique pointer. [...] Still, the fact it has been moved from should suggest to the user "this is in an unspecified state"

    You can simply add the ref-value qualifier && to extract_data:

    std::unique_ptr<foo> extract_data() && { return std::move(this->data); }

    This way, extract_data can only be called on rvalues of bar.

    bar bar_0{std::make_unique<foo>()};
    
    // does not compile, user may be tempted to use bar afterwards
    bar_0.extract_data();
    
    // compiles, user has to move bar first, unspecified state afterwards
    std::move(bar_0).extract_data();