My string class destructor implementation

Question

Having such simple program:

#include <iostream>
#include <string>
#include <windows.h>

using namespace std;
extern char MsgBuff[300];

class MyStr {
    string* strPtr;
public:

    // "normal" constructor
    MyStr(const string& strPtr) : strPtr(new string(strPtr)) {}

    // destructor
    ~MyStr() { 
        if(strPtr != NULL)
        delete strPtr; 
    }

    // copy constructor
    MyStr(const MyStr& x) : strPtr(x.strPtr) {
        OutputDebugStringA("copy constructor");
    }

    // move constructor
    MyStr(MyStr&& x) : strPtr(x.strPtr) {
        x.strPtr = nullptr;
        OutputDebugStringA("copy constructor");
    }

};


int main() {    

    MyStr foo("Exam");
    MyStr foo2 = foo;

    return 0;
}

The program throws an exception: Exception thrown: read access violation. As i invesigated it's caused by the destructor code - destroying these two objects (foo, foo2) we are freeing TWICE the same memory pointed by strPtr pointer.

How can this code be fixed to preserve the logic and avoid the exception?

Answer 1

A few things wrong with this code...

MyStr(const MyStr& x) : strPtr(x.strPtr) {
    OutputDebugStringA("copy constructor");
}

This code makes "shallow" copy of the class as it only assigns adresses to existing object instead of creating a new one. This is the main problem, because as main() goes out of scope destructors will be called on all initialized objects. First ~foo will be called. "Succesfully". Then ~foo2 will be called and as it is still a valid object destructor will be called.

if (strPtr != NULL)

will pass, because nowhere in your code do you set strPtr to "nullptr" and so delete on uninitialized object will be called. This will cause the memory access violation.

Few things to keep in mind:

1. Please use std::string as much as possible. (people that implement this know what they're doing)
2. Never ever use raw pointers unless absolutely necessary. (this is just realy ugly way of doing things with really no benefits. Use std::shared_ptr and std::unique_ptr instead.)
3. Always set pointer to NULL after calling delete on it. (This goes without saying. Dont leave objects set to invalid adresses.)
4. NEVER.. use extern and/or global variables NEVERR!! (This just shows bad code design/structure)
5. Also this is not "bad" in "main" cpp file, but try to avoid using "using namespace std;". This will save you some headache when working with multiple namespaces.

Now for the "fixed" code part.. I assume you want to do a wrapper for string, so here you go:

#include <iostream>
#include <memory>
#include <string>
#include <windows.h>

class MyStr {
    std::shared_ptr<std::string> m_ptrStr;
public:

    // "normal" constructor
    MyStr(const std::string& strPtr) : m_ptrStr(std::make_shared<std::string>(strPtr)) {}

    // destructor
    ~MyStr() { }

    // shallow copy constructor (you can do this since you have "smart" pointer)
    MyStr(const MyStr& x) : m_ptrStr(x.m_ptrStr) { 
        OutputDebugStringA("copy constructor");
    }

    // move constructor
    MyStr(MyStr&& x) noexcept : m_ptrStr(std::move(x.m_ptrStr)) {
        OutputDebugStringA("move");
    }

};


int main() {

    MyStr foo("Exam");
    MyStr foo2 = foo;

    return 0;
}

Answer 2

The problem in the code is with the copy ctor.

// copy constructor
    MyStr(const MyStr& x) : strPtr(x.strPtr) {
        OutputDebugStringA("copy constructor");
    }

Here you don't create a shallow copy of the string.

The solution is doing one of the following:

1. If memory and creation time is not an issue, create a deep copy by creating a new string and putting it inside of MyStr.
2. Using std::shared_ptr<std::String> instead of raw std::string*, which is a bit wasteful in my opinion.
3. Using copy-on-write approach. On copy, you don't create a new string, but rather another reference to the same resource, and as far as I know, this used to be the approach used by std::string. (you can read more about it on)

Another issue is using std::string since it is already doing exactly what you are trying to accomplish. If you want to make your own implementation, using raw pointers, use char * and not std::string *.

Answer 3

Presumably you're doing this to experiment with raw pointers as opposed to needing to house a string.

When using naked pointers, you need to properly implement copy constructors and assignment operators such that you perform a "deep copy" of the addressed data.

Whenever I do this, I write a "clone" method that would in your case perform:

void clone (MyStr const& src)
{
    strPtr = new string(*src.strPtr);
}

Also I would recommend using the "free-and-nil" idiom to avoid double-deletion:

if (srcPtr)
{
    delete srcPtr;
    srcPtr = nullptr;
}

I recommend the Book "Professional C++" by Marc Gregoire which covers this kind of detail. I own copies of the 3rd and 4th editions. https://www.amazon.co.uk/Professional-C-Marc-Gregoire/dp/1119421306