Note that the basis of the question is to use two malloc()s...while suggesting not using malloc() at all is perfectly valid and leads to better design, this is not the point of the question.

===== Here comes the question itself =====

Say I am stuck with the following class and cannot switch to features such as vector<int>, unique_ptr<int>, etc, while not "modern", it should still work without leaking any memory, whether or not malloc() succeeds:

class Foo {
    private:
        int *ptr;
    public:
        Foo () {
            this->ptr = (int*)malloc(sizeof(int) * ARR_SIZE);
            if (this->ptr == NULL) {
                std::bad_alloc exception;
                throw exception;
            }
        }
        ~Foo () {
            free(this->ptr);
        }

};

The question appears if I need to malloc() twice within the same constructor:

class Foo {
    private:
        int *ptr0;
        int *ptr1;
    public:
        Foo () {
            this->ptr0 = (int*)malloc(sizeof(int) * ARR_SIZE_0);
            if (this->ptr0 == NULL) {
                std::bad_alloc exception;
                throw exception;
            }
            this->ptr1= (int*)malloc(sizeof(int) * ARR_SIZE_1);
            if (this->ptr1== NULL) {
                free(this->ptr0); // QUESTION: Is this needed and does it follow the best practice?
                std::bad_alloc exception;
                throw exception;
            }
        }
        ~Foo () {
            free(this->ptr0);
            free(this->ptr1);
        }

};

I am aware that it could be more advisable in the 2nd case that we create two classes which wrap one pointer each, so the principle of RAII can be thoroughly followed and the above kind of "C-style" free() in constructor is not needed.

The question is, say, for whatever reason, I must have two malloc()s in the constructor, is my design good enough (i.e., not leaking memory and not too verbose)?

CodePudding user response：

Here is a RAII-enabled version.

struct Free
{
   void operator()(void* p) const { std::free(p); }
};

template <typename T, typename Deleter = Free>
std::unique_ptr<T, Deleter> raii_malloc()
{
   T* obj = static_cast<T*>(std::malloc(sizeof(T)));
   if (obj == nullptr) throw std::bad_alloc();
   return std::unique_ptr<T, Deleter>(obj);
}

template <typename T, typename Deleter = Free>
std::unique_ptr<T[], Deleter> raii_malloc_array(size_t size)
{
   T* obj = static_cast<T*>(std::malloc(sizeof(T) * size));
   if (obj == nullptr) throw std::bad_alloc();
   return std::unique_ptr<T[], Deleter>(obj);
}

template <typename T>
using fptr = std::unique_ptr<T, Free>;

Now your class looks like this:

class Foo
{
      fptr<int[]> ptr0;
      fptr<int[]> ptr1;
   public:
      Foo() : ptr0{raii_malloc_array<int>(ARR_SIZE_0)},
              ptr1{raii_malloc_array<int>(ARR_SIZE_1)}
      {}
      // no destructor needed
};

Note that this version is non-copyable. If you need copying, add a custom copy constructor and/or assignment operator (still no destructor needed).

You can use this with C objects that contain internal pointers or other resources that need to be freed, you just need to supply a different deleter instead of Free.

CodePudding user response：

1. Use std::vector or std::unique_ptr to help manage memory.
2. Don't use malloc unless you must.
3. Make sure your class is either safe to copy/move, or not copyable/movable (i.e., deleted constructor, operator=).
4. Think about how likely you are to handle an out-of-memory case anyway (are you going to open a file and log it while you're out of system memory?), and perhaps just terminate if the allocation fails.

CodePudding user response：

A rather simple way to avoid the issue would be to allocate the necessary memory for your member variables in one go.

class Foo
{
    public:
        Foo()
        {
            ptr0 = new int[ARR_SIZE0   ARR_SIZE1];
            ptr1 = ptr0   ARR_SIZE0;
        }

        ~Foo()
        {
            delete ptr0[];
        }

        // PLEASE insert the rest of the
        // necessary constructors
        // and assignment operators...

    private:
        int * ptr0;
        int * ptr1;
};

Using new instead of malloc here because, if you have to do manual allocations, at least do them using the C construct.