Possible alternatives to copy constructors

Question

In my C++ project I am relying on some libraries that do memory management for me. I make wrapper classes, for ease of use and memory safety, for example the class below. Note that this is a much simplified example, to demonstrate my problem.

#include <library>

class Wrapper {
private:
    lib_type* data;
public:
    Wrapper() : data(library_new()) {
    }
    Wrapper(const Wrapper& orig) = delete;
    ~Wrapper() {
        library_free(data);
    }
    const lib_type* getData() const {
        return data;
    }
    /* ... */
    /* Lots of functions for using the wrapped object */
};

I needed to delete the copy constructor, because otherwise a copy going out of scope would invalidate the original object. Not being allowed to have copies makes the object very impractical to use - everywhere it is used I now need to hold a reference, and I need to manage the object centrally, which partially defeats the purpose of the wrapper class.

Possible solutions I have tried/thought of:

• Copying the data. This is normally not an option though, because it is either not allowed by the library or the data is huge.
• Move constructors. I tried solving the lack of a copy constructor by using a move constructor, but depending on the implementation, either this did not solve the problem or it became effectively a copy constructor, reintroducing the related problems.
• Smart pointers. Then I realized it might be solved by using smart pointers to the wrapper instead of references, as this removes the need to maintain a central copy.
• Wrapping a smart pointer. Or I could wrap a smart pointer to the data instead of a raw pointer. This makes the implementation of the wrapper a little bit more complicated, but also makes it easier to use, giving the classes using it a cleaner interface.

My attempt at the second smart pointer solution applied to the example above:

#include <library>

class Deleter {
public:
    void operator()(lib_type* p) const {
        library_free(p);
    }
};

class Wrapper {
private:
    shared_ptr<lib_type> data;
public:
    Wrapper() : data(library_new(), Deleter()) {
    }
    Wrapper(const Wrapper& orig) : data(orig.data) {
    }
    ~Wrapper() {
    }
    const lib_type* getData() const {
        return *data;
    }
    /* ... */
    /* Lots of functions for using the wrapped object */
};

Now for the concrete question: is this a good solution? Or are there other, perhaps better solutions?

I am particularly worried about the getData implementation; whether I should return a copy of the shared pointer or a naked pointer and why.

Answer 1

I think these implementations are reasonable and a generally good solution. Adding an appropriate move constructor and move assignment may help deal with your copy concerns - the default should be appropriate with the shared wrapper.

Some may argue (or advise) that you do not need to wrap the Standard Library facilities that you use here; whilst this is true, the semantics of the getData() function may be quite specific for your target code - using the library facility or wrapping it is really a design tradeoff.

You may need a few tweaks on the assignment operators; and possibly be more explicit on the other special member functions (I would favour this here - it makes the intent of the code clearer). By way of example;

#include <library>

class UniqueWrapper {
private:
    lib_type* data;
public:
    UniqueWrapper() : data(library_new()) {}
    UniqueWrapper(const UniqueWrapper&) = delete;
    UniqueWrapper& operator=(const UniqueWrapper&) = delete;
    UniqueWrapper(UniqueWrapper&&) = delete;
    UniqueWrapper& operator=(UniqueWrapper&&) = delete;

    ~UniqueWrapper() {
        library_free(data);
    }
    const lib_type* getData() const {
        return data;
    }
};

And as a shared wrapper;

#include <library>

class Deleter {
public:
    void operator()(lib_type* p) const {
        library_free(p);
    }
};

class SharedWrapper {
private:
    shared_ptr<lib_type> data;
public:
    SharedWrapper() : data(library_new(), Deleter()) {}

    SharedWrapper(const SharedWrapper& orig) = default;
    SharedWrapper& operator=(const SharedWrapper& orig) = default;
    SharedWrapper(SharedWrapper&& orig) = default;
    SharedWrapper& operator=(SharedWrapper&& orig) = default;

    ~SharedWrapper() = default;

    const lib_type* getData() const {
        return data.get(); // a reference return could also be used.
    }
    const lib_type& getDataAlt() const {
        // alternative for a reference
        return *data;
    }
};

On your last question;

I am particularly worried about the getData implementation; whether I should return a copy of the shared pointer or a naked pointer and why?

When you expose some of the internals of an object, you give up some control over how that data is used - essentially this is not a bad thing, you just need to bear that in mind.

Having said that, there are ways you can make the code easy to use properly and hard to use incorrectly. Returning a const in this case would probably be advised, since the method is const - it is saying to the client of your code, don't change it. If changes are to be allowed, then a non-const version is also required.

The wrapper class you have is there to manage the resource, so I would advise returning a reference (hence for observation) over a pointer; a pointer can be a nullptr so if there is an "optional" nature to the value, a nullptr pointer can be used to indicate that.

If the shared and const-ness of the return type is to be maintained, David Hammen's solution of shared_ptr<const lib_type> is neat.

Answer 2

Just use shared_ptr directly, with your custom deleter. Maybe typedef it if you prefer.

This way you get correct move & copy constructors and assignment operators with no typing. You also get weak_ptr for free, if you want it.

Unless your code will add some actual functionality - or at least an interface compatible with some external requirement you didn't mention - the best thing you can possibly do is just not write it in the first place.

Look at Niall's SharedWrapper for a good example: unless you need that getData signature, it's a lot of code for no actual benefit. (This isn't a criticism: that was clearly written on the assumption that you do need the getData signature).

Answer 3

Note: This answer addresses the final part of the question. The bulk of the question has already been nicely addressed in Niall's answer.

I am particularly worried about the getData implementation; whether I should return a copy of the shared pointer or a naked pointer and why.

Suppose some code calls getData() and saves that pointer in some persistent object, and that the last remaining Wrapper object that references this pointer goes out of scope before this persistent object does. That persistent object now contains an invalid pointer.

To solve this problem, it would be much better to return the shared pointer. This runs into another problem, which is that your getData() currently returns a pointer to const lib_type. Returning a const shared_ptr<lib_type> will not do what you want, for two reasons. One is that this makes it possible for the caller of getData() to modify the data, something you don't want to happen. The other is that const is meaningless; it's like returning a const int. The compiler ignores that const. Instead, use

shared_ptr<const lib_type> getData() const {
    return data;
}

The above takes advantage of the fact that a shared_ptr<some_type> automatically converts to a shared_ptr<const some_type>. The reverse (converting a shared_ptr<const some_type> to a shared_ptr<some_type>) is illegal (which is presumably the behavior you want).

One last remark: Those type names can get long and clunky. Presumably you are using std::shared_ptr rather than shared_ptr, and lib_type is presumably something like some_namespace::AMoreDescriptiveNameThanLibType. To specify the type of the value received from a call to getData, a user of your code would have to write

std::shared_ptr<const some_namespace::AMoreDescriptiveNameThanLibType> data =
    wrapper.getData();

One way around this is to use auto:

    auto data = wrapper.getData();

Another option is to provide some type definitions (or some type aliases) in your definition of Wrapper:

class Wrapper {
public:
    typedef std::shared_ptr<lib_type> dataT;
    typedef std::shared_ptr<const lib_type> const_dataT;
    // Rest of code elided
};

Then your users only have to type

Wrapper::const_dataT data = wrapper.getData();