Search code examples
c++raii

C++ RAII - Do I have this right?


I'm new to C++ and am new to using RAII for deleting allocated memory. I wrote this code as a sample of what it would take to automatically allocate and later delete a character array. I know there's a string class out there, but thought I'd start with the older stuff first. Does this sample look correct? And is it the way others would create a string and delete it automatically?

#include <iostream>

using namespace std;

class StringGetter
{
    private:
        char* theString;

    public:
        StringGetter(char* name)
        {
            this->theString = new char[1024];

            strcpy_s(this->theString, strlen(name) + 1, name);
        }

        char* GetString()
        {
            return this->theString;
        }

        ~StringGetter()
        {
            delete[] this->theString;
        }
};

int main()
{
    char* response;

    {
        StringGetter getter("testing");
        response = getter.GetString();
    }

    cin.get(); // pauses the console window
}

Solution

  • It looks like you get the main idea, but there's a couple things worth mentioning.

    1) As @chris mentioned, you're forgetting your copy constructor, copy assignment operator, move constructor, and move assignment operator. Copy should be either manually written or disabled, move can be defaulted. (aka You've not followed the rule of 5)

    2) Prefer to use std::unique_ptr to represent ownership. It's already done all of the hard work for you. By keeping your wrapped string in a std::unique_ptr the default versions of the copy/move special functions will preserve correctness (though you'll still want to implement the copy operations if you want them to be enabled).

    Here's what this might look like:

    class StringGetter {
      public:
        explicit StringGetter(char* name) {
          strSize = strlen(name);
          str = std::unique_ptr<char[]>(new char(strSize+1));
          std::copy_n(name, strSize + 1, str.get());
        }
    
        StringGetter(const StringGetter& other) {
          strSize = other.strSize;
          str = std::unique_ptr<char[]>(new char(strSize+1));
          std::copy_n(other.str.get(), strSize + 1, str.get());
        }
    
        StringGetter(StringGetter&& other) = default;
    
        StringGetter& operator=(const StringGetter& rhs) {
          auto temp = rhs;
          swap(temp);
          return *this;
        }
    
        StringGetter& operator=(StringGetter&& rhs) = default;
    
        const char* getString() const noexcept {
          return str.get();
        }
    
        void swap(StringGetter& other) {
          std::swap(str, other.str);
          std::swap(strSize, other.strSize);
        }
      private:
        std::unique_ptr<char[]> str;
        int strSize;
    };
    

    Miscellaneous items:

    1) Note that std::unique_ptr handles the RAII. When I replace 'str' in the copy constructor, it deletes the old string automatically.

    2) I size the dynamically allocated memory to match the input string. This prevents overflows/wasted memory.

    3) The constructor is explicit. This prevents undesirable conversions. Rule of thumb is to use the explicit keyword on all single argument constructors.

    4) I made getString constant so that you can still call it on const instances of the class.

    5) I replaced the str-family copy function with std::copy_n. It's more general and can avoid certain pitfalls in obscure cases.

    6) I used the copy-swap idiom in the copy assignment operator. This promotes code reuse/avoids duplication.

    7) When C++14 comes out, replace the std::unique_ptr constructor call with std::make_unique for added exception-safety and to remove redundancy.