C++ Move Semantics - Wrapping Legacy C APIs

Question

I am working with a legacy C API under which acquiring some resource is expensive and freeing that resource is absolutely critical. I am using C++14 and I want to create a class to manage these resources. Here is the basic skeleton of the thing...

class Thing
{
private:
    void* _legacy;

public:
    void Operation1(...);
    int Operation2(...);
    string Operation3(...);

private:
    Thing(void* legacy) :
        _legacy(legacy)
    {
    }
};

This is not really the singleton pattern. Nothing is static and there may be many Thing instance, all managing their own legacy resources. Furthermore, this is not merely a smart-pointer. The wrapped pointer, _legacy is private and all operations are exposed via some public instance functions that hide the legacy API from consumers.

The constructor is private because instances of Thing will be returned from a static factory or named-constructor that will actually acquire the resource. Here is a cheap imitation of that factory, using malloc() as a place-holder for the code that would invoke the legacy API ...

public:
    static Thing Acquire()
    {
        // Do many things to acquire the thing via the legacy API
        void* legacy = malloc(16);

        // Return a constructed thing
        return Thing(legacy);
    }

Here is the destructor which is responsible for freeing the legacy resource, again, free() is just a placeholder ...

    ~Thing() noexcept
    {
        if (nullptr != _legacy)
        {
            // Do many things to free the thing via the legacy API
            // (BUT do not throw any exceptions!)
            free(_legacy);
            _legacy = nullptr;
        }
    }

Now, I want to ensure that exactly one legacy resource is managed by exactly one instance of Thing. I did not want consumers of the Thing class to pass instances around at will - they must either be owned locally to the class or function, either directly or via unique_ptr, or wrapped with a shared_ptr that can be passed about. To this end, I deleted the assignment operator and copy constructors...

private:
    Thing(Thing const&) = delete;
    void operator=(Thing const&) = delete;

However, this added an additional challenge. Either I had to change my factory method to return a unique_ptr<Thing> or a shared_ptr<Thing> or I had to implement move semantics. I did not want to dictate the pattern under which Thing should be used so I chose to add a move-constructor and move-assignment-operator as follows...

    Thing(Thing&& old) noexcept : _legacy(old._legacy)
    {
        // Reset the old thing's state to reflect the move
        old._legacy = nullptr;
    }

    Thing& operator= (Thing&& old) noexcept
    {
        if (&old != this)
        {
            swap(_legacy, old._legacy);
        }
        return (*this);
    }

With this all done, I could use Thing as a local and move it about...

    Thing one = Thing::Acquire();
    Thing two = move(one);

I could not break the pattern by attempting to commit self-assignment:

    Thing one = Thing::Acquire();
    one = one;                      // Build error!

I could also make a unique_ptr to one...

    auto three = make_unique<Thing>(Thing::Acquire());

Or a shared_ptr ...

    auto three = make_shared<Thing>(Thing::Acquire());

Everything worked as I had expected and my destructor ran at exactly the right moment in all my tests. In fact, the only irritation was that make_unique and make_shared both actually invoked the move-constructor - it wasn't optimized away like I had hoped.

First Question: Have I implemented the move-constructor and move-assignment-operator correctly? (They're fairly new to me and this will be the first time I have used one in anger.)

Second Question: Please comment on this pattern! Is this a good way to wrap legacy resources in a C++14 class?

Finally: Should I change anything to make the code better, faster, simpler or more readable?

Answer 1

You should wrap your Thing in a smart pointer, then you won't need to worry about the copy and move semantics.

class Thing
{
private:
    void* _legacy;

public:
    void Operation1(...);
    int Operation2(...);
    string Operation3(...);

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

    static std::shared_ptr<Thing> acquire() {
        return std::make_shared<Thing>();
    }

private:
    Thing() : _legacy(malloc(16)) {
         // ...
    }

    ~Thing() {
        free(_legacy);
    }
};

Similarly, you can do it with unique_ptr:

std::unique_ptr<Thing> acquire() {
    return std::make_unique<Thing>();
}

You seemed to imply that you want to have only one instance of this thing, though even in your solution you did not attempt to do anything like that. For that you need static variables. Though remember that in this case your resource will only be freed after your main() function exits. For example:

static std::shared_ptr<Thing> acquire() {
    static std::shared_ptr<Thing> instance;
    if (!instance) {
        instance = std::make_shared<Thing>();
    }
    return instance;
}

Or the unique_ptr version:

static Thing& acquire() {
    static std::unique_ptr<Thing> instance;
    if (!instance) {
        instance = std::make_unique<Thing>();
    }
    return *instance;
}

Or, you can use weak_ptr to acquire one instance program-wide, which is freed when nobody uses it. In this case you won't be able to use unique_ptr for this purpose. This version will recreate the object if it was freed then needed again.

static std::shared_ptr<Thing> acquire() {
    static std::weak_ptr<Thing> instance;
    if (instance.expired()) {
        instance = std::make_shared<Thing>();
    }
    return instance.lock();
}