C++ RAII helper classes for malloc arrays and files

Question

I'm going to use these classes in a program I'm working on, so I want to see if they're correct or could be improved. https://pastebin.com/qx7ccteT

#include <cstdio>
#include <cstdlib>

class FileHelper
{
public:
    FILE* f = NULL;
    char buffer[8192];
    int bufferPosition = 0;
    int bytesRead = 0;

    FileHelper(const char* filePath)
    {
        f = std::fopen(filePath, "rb");
        if (!f)
        {
            throw "FileHelper fopen failure in constructor";
        }
    }

    ~FileHelper()
    {
        if (f)
        {
            std::fclose(f);
            f = NULL;
        }
    }

    bool getCharacter(char& ch)
    {
        if (bufferPosition == 8192 || bufferPosition == bytesRead)
        {
            bufferPosition = 0;
            bytesRead = fread(buffer, sizeof(char), 8192, f);
            if (!bytesRead)
            {
                return false;
            }
        }
        ch = buffer[bufferPosition++];
        return true;
    }
};

template <typename T>
class ArrayHelper
{
public:
    T* arr = NULL;
    size_t capacity = 0;
    size_t arrPosition = 0;

    ArrayHelper()
    {
        arr = (T*)std::malloc(8 * sizeof(T));
        if (!arr)
        {
            throw "ArrayHelper malloc failure in constructor";
        }
        capacity = 8;
    }

    ~ArrayHelper()
    {
        if (arr)
        {
            std::free(arr);
            arr = NULL;
        }
    }

    void append(T value)
    {
        if (arrPosition == capacity)
        {
            // python's overallocation
            int newCapacity = ((size_t)(capacity + 1) + ((capacity + 1) >> 3) + 6) & ~(size_t)3;
            T* temp = arr;
            arr = (T*)std::realloc(arr, newCapacity * sizeof(T));
            if (!arr)
            {
                arr = temp;
                throw "ArrayHelper realloc failure in append method";
            }
            capacity = newCapacity;
        }
        arr[arrPosition++] = value;
    }

    void clear()
    {
        arrPosition = 0;
    }

    // copies given to PointerArrayHelper
    T* copy()
    {
        if (!arrPosition)
        {
            return NULL;
        }
        T* arrCopy = (T*)std::malloc(arrPosition * sizeof(T));
        if (!arrCopy)
        {
            throw "ArrayHelper malloc failure in copy method";
        }
        for (size_t i = 0; i < arrPosition; i++)
        {
            arrCopy[i] = arr[i];
        }
        return arrCopy;
    }
};

template <typename T>
class PointerArrayHelper
{
public:
    T* arr = NULL;
    size_t capacity = 0;
    size_t arrPosition = 0;

    PointerArrayHelper()
    {
        arr = (T*)std::malloc(8 * sizeof(T));
        if (!arr)
        {
            throw "PointerArrayHelper malloc failure in constructor";
        }
        capacity = 8;
    }

    ~PointerArrayHelper()
    {
        if (arr)
        {
            for (size_t i = 0; i < arrPosition; i++)
            {
                if (arr[i])
                {
                    std::free(arr[i]);
                    arr[i] = NULL;
                }
            }
            std::free(arr);
            arr = NULL;
        }
    }

    template <typename U>
    void append(ArrayHelper<U>& arrHelper)
    {
        if (arrPosition == capacity)
        {
            // python's overallocation
            int newCapacity = ((size_t)(capacity + 1) + ((capacity + 1) >> 3) + 6) & ~(size_t)3;
            T* temp = arr;
            arr = (T*)std::realloc(arr, newCapacity * sizeof(T));
            if (!arr)
            {
                arr = temp;
                throw "PointerArrayHelper realloc failure in append method";
            }
            capacity = newCapacity;
        }
        arr[arrPosition++] = arrHelper.copy();
    }

    void shrink()
    {
        T* temp = arr;
        arr = (T*)std::realloc(arr, arrPosition * sizeof(T));
        if (!arr)
        {
            arr = temp;
            throw "PointerArrayHelper realloc failure in shrink method";
        }
        capacity = arrPosition;
    }
};

int main()
{
    try
    {
        FileHelper fh("test.txt");
        PointerArrayHelper<char*> ptrArrHelper;
        ArrayHelper<char> arrHelper;
        char ch = '\0';

        while (fh.getCharacter(ch))
        {
            if (ch == '\n')
            {
                arrHelper.append('\0');
                ptrArrHelper.append(arrHelper);
                arrHelper.clear();
            }
            else
            {
                arrHelper.append(ch);
            }
        }
        if (ch != '\n')
        {
            arrHelper.append('\0');
            ptrArrHelper.append(arrHelper);
        }

        ptrArrHelper.shrink();
        for (size_t i = 0; i < ptrArrHelper.capacity; i++)
        {
            std::printf("%s\n", ptrArrHelper.arr[i]);
        }
    }
    catch (char const* e)
    {
        std::printf("%s\n", e);
    }

    return 0;
}

Answer 1

Overview:

I would note that there a great working versions of these in C++. So you should not do them yourself.

But if we consider this an exercise in learning how to use C++ then lets have a look.

Big Issues:

• FileHelper: Does not implement the rule of 3.
• ArrayHelper: Does not implement the rule of 3
• ArrayHelper: Does not work for objects with constructor / destructor

Minor Issues:

• FileHelper: Does not implement move semantics.
• ArrayHelper: Does not implement move semantics.
• ArrayHelper: Introduces extra copy on insertion.
• ArrayHelper: Does not have move insertion.
• ArrayHelper: Does not support emplacing objects

Code Review:

Quick note: FILE* is already buffered.

    char buffer[8192];

---

Sure that is a good constructor.

    FileHelper(const char* filePath)

I can see a lot of situations where you don't know the filePath immediately and may want to create the object then open it later. So haveing a default constructor may be useful.

---

    ~FileHelper()
    {
        if (f)
        {
            std::fclose(f);
        
            // This is a bad idea.
            f = NULL;
        }
    }

The reason it is a bad idea is that it is not necessary (the object is about to not exist the value in the memory it used to occupy does not matter, so waste of an instruction.

But the real reason that it is bad idea is that some debugging environments will help you find double deletes of pointers. If you don't know the rule of three and have created a copy of the object and the constructor is called twice on the same pointer then the debug environment will help you find this situation (but not if you null out the pointer it is using to help you).

---

Talking about the rule of three. You did not follow it.

If you do not define a copy constructor or copy assignment operator then the compiler will generate them for you. These will normally do the correct thing. APART from when you have an owned resource in the object that is cleaned up by the destructor. Then it will fail.

Your object has an owned resource FILE* that you clean up in the destructor. As a result the default copy constructor and copy assignment will not work.

{
    FileHelper  test("Bob");
    FileHelper  test2(test1);
}
// Blows up here because you have a double delete on the same FILE*

---

    bool getCharacter(char& ch)
    {
        if (bufferPosition == 8192 || bufferPosition == bytesRead)
            ^^^^^^^^^^^^^^^^^^^^^^^^^
            // This seems like it is not needed.
            // If you fill the buffer than would bytesRead not be 8192?

Also there are two different types of situation here:

            if (!bytesRead)
            {
                return false;
            }

There is End Of File and there is an error condition. End of file is pretty normal but other situations seem like they would need some extra ability to check for.

---

In C++ we use nullptr not NULL.

    T* arr = NULL;

The macro NULL has issues while nullptr solves these.

---

You have an issue with you destructor if the type T has a destructor as the array does not clean up the objects by calling it. Thus makes me think you don't call the constructor of T either.

You can limit your ArrayHelper to only support objects that don't have constructor/destructor then you would be fine.

    ~ArrayHelper()
    {
        // Add a static assert here that checks that T
        // does not have a destructor.
        if (arr)
        {
            // Or make sure you call the destructor of all constructed
            // members of the array before you release the memory.
            std::free(arr);

            // Like your file helper this is a waste of time.
            // And a bad idea as it stops the system from helping you
            // debug.
            arr = NULL;
        }
    }

---

I would break the reallocation part of this function into its own named function. That will make your code easier to read.

Passing the parameter by value:

    void append(T value)

This forces a copy of the parameter (this can be expensive for some types of T). Note: It is OK to use a value here as a parameter if you know about and use move semantics. BUT you don't so you should be passing the parameter by reference.

---

            // python's overallocation
            int newCapacity = ((size_t)(capacity + 1) + ((capacity + 1) >> 3) + 6) & ~(size_t)3;

OK. That looks complicated. There should be a comment here. You don't need to explain the exact meaning but at least have a link so people can follow the link the explanation.

---

You correctly handle failed re-allocation.

            T* temp = arr;
            arr = (T*)std::realloc(arr, newCapacity * sizeof(T));
            if (!arr)
            {
                arr = temp;
                throw "ArrayHelper realloc failure in append method";
            }
            capacity = newCapacity;

But I think it can be written more simply as:

            T* temp = (T*)std::realloc(arr, newCapacity * sizeof(T));
            if (!temp)
            {
                throw "ArrayHelper realloc failure in append method";
            }
            arr = temp;
            capacity = newCapacity;

---

Sure.

        arr[arrPosition++] = value;

But if T has a constructor the object is not correctly constructed. You need to construct the value in place.

        new (arr + arrPosition) T(value);
        ++arrPosition;

---

Like the destructor you need to destroy objects that are no longer in the array.

    void clear()
    {
        arrPosition = 0;
    }

---

Note this will not correctly copy objects that have constructors.

        T* arrCopy = (T*)std::malloc(arrPosition * sizeof(T));