Wrapper for dynamically-allocated memory block

Question

I have built a single header-only memory class (just a C++ wrapper around a dynamically-allocated pointer and a size) that supports copy and move semantics (I hope I implemented them correctly) for a game I'm building. Its use case is to wrap the raw contents of assets imported (from any source) before processing them. So there is:

#include <cstdint>
#include <algorithm>

class Memory
{
    uint8_t *ptr;
    size_t size;

public:
    inline Memory() : ptr(nullptr), size(0) {}
    inline Memory(size_t size) : ptr(new uint8_t[size]), size(size) {}
    inline Memory(const Memory &other) : Memory(other.size)
    {
        std::copy(other.ptr, other.ptr+size, ptr);
    }
    inline Memory(Memory &&other) : Memory() { swap(*this, other); }

    inline Memory& operator=(Memory other)
    {
        swap(*this, other);
        return *this;
    }

    inline ~Memory() { delete[] ptr; }

    inline const void* get_ptr() const { return ptr; }
    inline void* get_ptr() { return ptr; }
    inline size_t get_size() const { return size; }

    friend inline void swap(Memory &m1, Memory &m2)
    {
        using std::swap;
        swap(m1.ptr, m2.ptr);
        swap(m1.size, m2.size);
    }
};

Is there something I forgot to add? Are there any errors? Can it be improved in any way?

Answer 1

Code Review

Namespace

It would be nice to put your code inside its ownnamespace to avoid potentially collisions. Memory would seem like a good general name that would be reused by other people at some point.

Style

This is minor.
But most C++ developers put the * beside the type not the name. This is the reverse of the standard C style which is to put the * beside the name.

    uint8_t *ptr;

C++ is all about the type of the object. Putting the star with the type helps to correctly associate the type as a pointer.

Don't use inline unless you are required.

Anything inside the class declaration also automatically marked inline. Also the keyword inline has little to no affect on the compiler (ie it is ignored by every modern compiler) and its decision to actually inline the code.

    inline Memory() : ptr(nullptr), size(0) {}

The only time it is needed is when a method is defined in a header file included in multiple compilation units. Then it is used by the linker to note that the one definition rule is not being broken.

Constructors

All the constructors are good. Though I did have to double check the copy constructor.

    inline Memory() : ptr(nullptr), size(0) {}
    inline Memory(size_t size) : ptr(new uint8_t[size]), size(size) {}
    inline Memory(const Memory &other) : Memory(other.size)
    {
        std::copy(other.ptr, other.ptr+size, ptr);
    }
    inline Memory(Memory &&other) : Memory() { swap(*this, other); }

BUT: The move constructor should also be marked noexcept (except that your move constructor will not throw). This does not do anything for the class itself. But it allows for optimizations in the standard library if you put your class in a container.

Example: When resizing a std::vector<Memory> it will use a noexcept move constructor if it exists otherwise it has to fall back to the copy constructor to provide that strong exception guarantee.

Assignment

This is wrong. I missed the copy caused by pass by value in the parameter

    inline Memory& operator=(Memory other)
    {
        swap(*this, other);
        return *this;
    }

The copy assignment operator has the semantics of doing move assignment. I don't think this is a good idea. You might get some surprised people that don't see that coming when there Memory object changes under the noses when they do an assignment.

I would convert this to a copy operator and then create a move assignment operator (don't forget to mark the move assignment as noexcept).

I would still add a move assignment operator (since you have move constructor).

Answer 2

I would like to suggest a quite radical refactoring of your code.

using Memory = std::vector<char>;

The standard library already has everything you need here.

Of course, it might still be a good learning opportunity to implement things yourself but as I understand your question, you're actually trying to get a job done here, rather than intentionally reimplementing existing functionality as an acedemic exercise.