HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

MallocRaii implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationmallocraiistackoverflow

Problem

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

Solution

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

Code Snippets

bool valid() const {
    return ptr == nullptr;
}
operator bool() const { return ptr != nullptr; }
// or else
bool operator==(std::nullptr_t) const { return ptr != nullptr; }
int main() {
    MallocRaii m(10);
    MallocRaii n(10);
    m = std::move(n);  // infinite loop
}
if(this->ptr != nullptr)
        free(this->ptr);
~MallocRaii() { free(ptr); }

Context

StackExchange Code Review Q#97647, answer score: 4

Revisions (0)

No revisions yet.