patterncppMinor
MallocRaii implementation
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:
Use it like this:
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 couldmallocanother 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 memorySolution
You have:
I think you meant
Try the following program:
This is because
A minor nit:
You're missing a space after
You make
Prefer to expose the value of the pointer (but not an actual reference to it) via an accessor, traditionally spelled
Finally, be aware that you could always just use
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 eitheroperator 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.