MallocRaii implementation

Question

I'm playing around with RAII a bit lately and I wan't to know if/how I can improve this (quite simple but very helpful) class.

A word to two decisions I've made and why:

• No error handling in construct. I don't think throwing a exception would be appropriate so it's up to the user to check valid().
• No copy constructor. I don't see how this could be useful. Since two objects can't share the same ptr, a copy could malloc another chunk of memory with same size and copy the old one, but I don't know if this would be useful at all.

template<typename T>
class MallocRaii {
public:
    MallocRaii(size_t size) : ptr(nullptr) {
        this->ptr = (T*)malloc(sizeof(T) * size);
    }

    ~MallocRaii() {
        if(this->ptr != nullptr)
            free(this->ptr);
    }

    MallocRaii(const MallocRaii& other) = delete; //Copy constructor
    MallocRaii& operator=(const MallocRaii& other) = delete; //Copy assignment

    MallocRaii(MallocRaii&& other) { //move constructor
        std::swap(*this, other);
    }

    MallocRaii& operator=(MallocRaii&& other) { //Move assignment
        std::swap(*this, other);
    }

    bool valid() const {
        return ptr == nullptr;
    }

    T* ptr;
};

Use it like this:

MallocRaii<unsigned long> buffer(256);
some_c_api_read_pixel_from_file(buffer.ptr, 256);
//Reads 256 pixels with 4 components from file,
// the class allocated 1024 bytes (256 * sizeof unsigned long)
do_something_with_the_data(buffer.ptr);
return; //class free's memory

Answer 1

You have:

bool valid() const {
    return ptr == nullptr;
}

I think you meant != there. Regardless, you probably don't need this method; and if you must have it, consider spelling it either

operator bool() const { return ptr != nullptr; }
// or else
bool operator==(std::nullptr_t) const { return ptr != nullptr; }

---

Try the following program:

int main() {
    MallocRaii m(10);
    MallocRaii n(10);
    m = std::move(n);  // infinite loop
}

This is because std::swap is implemented in terms of move-assignment, but you've chosen to implement move-assignment in terms of std::swap! Here are some slides on the subject (full disclosure: I wrote them).

---

A minor nit:

   if(this->ptr != nullptr)
        free(this->ptr);

You're missing a space after if; and you don't need the test anyway, because free(nullptr) is already guaranteed to do the right thing. Simply write

~MallocRaii() { free(ptr); }

---

You make ptr a public member variable. This allows anybody to come along and modify it:

MallocRaii m(10);
m.ptr = nullptr;  // oops! memory leak!

Prefer to expose the value of the pointer (but not an actual reference to it) via an accessor, traditionally spelled get().

T *get() const { return ptr; }

---

Finally, be aware that you could always just use std::unique_ptr<T, decltype(free)> instead of writing your own class; or at least that you could pretty easily implement your MallocRaii class in terms of unique_ptr. This would give you foolproof swap/move/no-copy boilerplate for free (via the Rule of Zero), and would be shorter as well.

Answer 2

Error Handling

No error handling in construct. I don't think throwing a exception would be appropriate so it's up to the user to check valid().

No. That is a bad choice.
If the constructor completes you should have a valid object. Using a method to check if an object is valid is just as bad as checking an error code. It is prone to not be used.

If your objects fails to construct throw an exception (std::runtime_error is a reasonable choice in most cases). If the user fails to catch the exception the application will exit (which is good; if they fail to check and fix errors then you have a bug so the application should exit when there is a bug).

Even in your own example code you forget to check:

MallocRaii<unsigned long> buffer(256);      // what happens if this failed.
                                            // your following code is going
                                            // to behave badly


some_c_api_read_pixel_from_file(buffer.ptr, 256);
//Reads 256 pixels with 4 components from file,
// the class allocated 1024 bytes (256 * sizeof unsigned long)
do_something_with_the_data(buffer.ptr);
return; //class free's memory

Constructor

Why are you using malloc?

MallocRaii(size_t size) : ptr(nullptr) {
    this->ptr = (T*)malloc(sizeof(T) * size);
}

OK. Its an example. Fine.
But it does not call the constructor of T. So you have a pointer to an array of T. But non of the members of T are in defined state. If you had called new then they would each be initialized.

Why not just call the malloc as part of initializer list. I don't see the point of making it null then setting the value. You are doing extra work for no gain.

Destructor

Even C no longer requires you to check for nullptr before freeing. Is it good practice to free a NULL pointer in C?. So there is no need for that test.

~MallocRaii() {
    if(this->ptr != nullptr)
        free(this->ptr);
}

Copy Semantics

Sure good call no copy.

MallocRaii(const MallocRaii& other) = delete; //Copy constructor
MallocRaii& operator=(const MallocRaii& other) = delete; //Copy assignment

Move Semantics

One small (VERY BIG) problem here.
The value of ptr is undefined in the new object (read random value). When you swap it with other you are putting an undefined value in other. When other goes out of scope you will call free() on some random pointer.

A constructor (no matter what type) should initialize all its members.

MallocRaii(MallocRaii&& other) { //move constructor
    std::swap(*this, other);
}

Also why are you using std::swap?. IF you are going to the effort of writting move you are going to need your own swap. Not sure that will even work.

MallocRaii& operator=(MallocRaii&& other) { //Move assignment
    std::swap(*this, other);
}

The main thing you forgot was that both move operations should be marked as noexcpt. If you don't do that then they may not be used in all situations in the standard libraries (but you must guarantee no exceptions).

Two Stage Initialization

Bad Idea. If an object goes into an invalid state then throw an exception.

bool valid() const {
    return ptr == nullptr;
}

Access to Members

Your member is public?

T* ptr;

A user can accidentally change that value.

 MallocRaii<int>   data(15);

 if (Data.ptr = nullptr) {
             ^^^  Accidental assignment.
                  But there are a hundred ways to accidentally screw up.

You should never provide public access to member variables. Always guard their use threw members.

I see your usage is:

do_something_with_the_data(buffer.ptr);
//  So I assume you are passing a T* pointer around a lot.

An easy way to do that is to use a conversion operator.

template<typename T>
class MallocRaii
{
     // If your object is used in any location that
     // a T* would normally be used. Then we auto convert
     // the object into a T* for you.
     operator T*        {return ptr;}
};

void some_c_api_read_pixel_from_file(unsigned long* data);
void do_something_with_the_dataunsigned long* data);
int f()
{
    MallocRaii<unsigned long> buffer(256);

    some_c_api_read_pixel_from_file(buffer, 256);  // buffer auto converted 
    do_something_with_the_data(buffer);

    return 1; //class free's memory
}