3

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.

  • 3
    It is generally a good solution. But you should be more specific (what exactly is lib_type ? what are its memory conventions?). Perhaps you need to use unique_ptr. – Basile Starynkevitch Aug 12 '15 at 10:35
  • There are various cases, this was just an abstract example. The concrete example which led me to finally asking here is an image loader, so the pointer points to a large unsigned char array and the library provides me with the size of the array as well. Another example is an OpenGL shader program. Here there is no data present as it is only present on the GPU, but glDeleteProgram needs to be called when I am done with it. In this case I suppose only the other smart pointer solution is possible. I don't see how unique_ptr can be of help, because it still won't allow copies. – Oebele Aug 12 '15 at 10:49
  • Do you need copies of the shader program? These things may be obvious to you, but they aren't to us, and you didn't really discuss your lifetime or sharing requirements in any detail. – Useless Aug 12 '15 at 13:21
  • @DavidHammen just a typo – Oebele Aug 12 '15 at 14:06
  • @Useless yes, different renderers may use it, and with copies based on shared pointers, the program will be deleted when all renderers don't need it anymore, which seems to be the best RAII-strategy to me. Otherwise I would need to maintain a central instance, which makes it unclear when to destroy it. – Oebele Aug 12 '15 at 14:08
4

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.

  • Wouldn't getData() return a lib_type in the second example (shared wrapper)? Dereferencing the shared_ptr would return the data, not a pointer. Maybe you meant data.get()? – LorToso Aug 12 '15 at 12:09
  • @LorToso. Yes, I copied that code over, I'll get that fixed. Thanks for that. – Niall Aug 12 '15 at 12:11
  • I don't understand what you mean with your second paragraph ("Some may argue..."). If I see it correctly the only changes other than the fix by LorToso you made were explicitly adding the default special members. Why is it better to do this explicitly? – Oebele Aug 12 '15 at 14:11
  • @Oebele. I mean "same may say...", the custom class may not be needed as it is a thin wrapper over std::shared_ptr. It is a personal preference, but given the RAII nature of the class I favour being explicit in the class definition of what is defined and what is deleted. I think that is makes the RAII class usage and intent clearer. C++ has rules on the special members, so it is not explicitly needed (and can be removed as personal preference). – Niall Aug 12 '15 at 14:22
  • @Oebele. There is not much that needs to change - I think you have a good solution. These are merely minor tweaks. – Niall Aug 12 '15 at 14:25
3

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).

  • In many cases there is a bit more to it than in the example. Additionally, I am okay with adding this amount of code, if that improves the ease-of-use and readability of the rest of the code. – Oebele Aug 12 '15 at 14:12
  • If you think shared_ptr is hard to use and hinders readability ... I strongly suggest you just get more familiar with shared_ptr and use it anyway, tbh. Unless you mean there's a bit more to your wrapper? Either way, I'd recommend shared_ptr as the default, and write your own wrapper only if you have some strong reason. – Useless Aug 12 '15 at 14:20
  • The main reason is because there is more to the wrapper. With readability I meant, for example, that a member of the type Image makes it intent a lot clearer than a member of the type shared_ptr<unsigned char> or unsigned char*. I believe this is called self-documentation. But again, main reason is that there is more to the class. – Oebele Aug 12 '15 at 14:27
  • 1
    I see. Well, since I can't comment on code I can't see, I still don't have any reason to prefer the wrapper over unique_ptr (and typedef is probably sufficient for documentation purposes). My general approach is that writing a custom wrapper is bad practice unless it does something, and I don't know what, if anything, yours does. – Useless Aug 12 '15 at 14:30
2

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();
  • I like the return type here - very neat. – Niall Aug 12 '15 at 13:55
  • What, effectively, does it mean if a shared_ptr is const? The contents cannot be changed, but that does not matter, because it is a copy, right? Does it also mean it can't be copied, as that changes the reference count? I only see downsides of doing that. consting the contents is nice though. – Oebele Aug 12 '15 at 14:01
  • A shared_ptr which has a template argument of const lib_type means that this smart pointer allows you to access an instance lib_type that is const-qualified (either by pointer or by reference), meaning that you cannot write to its public fields or call its non-const-qualified methods. – rwong Aug 12 '15 at 14:26
  • @rwong - The const that preceded shared_ptr<const lib_type> was indeed a typo. Thanks for catching it; I fixed it. – David Hammen Aug 12 '15 at 14:27
  • @DavidHammen so, the first const in your example should be removed? If so could you please update your answer to reflect that? – Oebele Aug 12 '15 at 14:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.