0

Whenever a the String Destructor is hit it Triggers a Breakpoint, i think i might be deleting the same variable twice or not assigning the right amount of memory during the manipulation of m_str

#include "String.h"
#include <iostream>

using namespace std;

String::String()
{
    m_str = nullptr;
}

String::String(const char* newStr)
{
    m_str = new char[strlen(newStr)+ 1];                    
    strcpy(m_str, newStr);                              
}

String::~String()
{
    if (m_str != nullptr)
    {
        delete[] m_str;
    }
}

void String::operator=(const String & myString)
{
    if (m_str != nullptr)
    {
        delete[] m_str;                      //Breakpoint Apears Here
    };
    m_str = new char[strlen(myString.m_str) + 1];
    m_str = myString.m_str;
}

void String::operator=(char* newStr)
{
    if (m_str != nullptr)
    {
        delete[] m_str;
    };
    m_str = new char[strlen(newStr) + 1];
    m_str = newStr;
}
}
  • 1
    And what is your question? – Mohit Jain Mar 25 '16 at 07:49
  • You *must* have a copy constructor, and for a resource containing type like this, a move constructor and assignment operator would be highly desirable. (Move constructor: `String(String&&rhs):m_str(rhs.m_str){rhs.m_str = nullptr;}'). – Martin Bonner supports Monica Mar 25 '16 at 08:04
  • check rule of three and rule of five – Giorgi Moniava Mar 25 '16 at 08:09
  • Another note: you don't need to check if the pointer is a nullptr, you can just call delete[] m_str; because you can delete nullptr's (it is just doing nothing then). And in your c'tor, you can directly initialize your member with String::String() : m_str(nullptr) or String::String(const char* newStr) : m_str(new char[strlen(newStr)+ 1]) etc. Btw, it might also be helpful to check other string implementations, for example std lib's implementation for that matter. – CppChris Mar 25 '16 at 08:43
  • Please complete your example, so that everyone can compile it. Make sure you don't include stuff that is not necessary though, it should be a minimal but complete example to be on-topic. – Ulrich Eckhardt Mar 25 '16 at 09:15

2 Answers2

0

You correctly use a strcpy to copy chars from original char array in String::String(const char* newStr), but in all other places in your code, you wrongly write:

m_str = new char[strlen(myString.m_str) + 1];
m_str = myString.m_str;

First line correctly allocates an array of correct size, but the second erases the pointer obtained by new so:

  • you get a memory leak, since the allocated memory has no pointer on it any longer and will never be freed
  • the m_str member points to the original char array. If it is later deleted you have a dangling pointer, and if you delete this instance first, you will try to delete the original char pointer in another instance making it dangling, or even try to delete a static or local array.

TL/DR: consistently copy the character array with strcpy (or std::copy)

manuell
  • 7,528
  • 5
  • 31
  • 58
Serge Ballesta
  • 143,923
  • 11
  • 122
  • 252
0

Assigning one const char * to another as you do e.g. In the assignment operators doesn't copy the string, it just copies the pointer. So instead of two separate strings, you now have two pointers, pointing to the same string.

So for one, you are leaking the newly created char array and second, you are trying to delete whatever has been passed to the assignment operator, which in your case is probably a string literal instead of something that was created via new. Also in the case of the copy assignment operator, you would end up with two String objects pointing to the same char array and thus double deletion.

To solve this Problem, just use strcpy as you did in the constructor.

MikeMB
  • 20,029
  • 9
  • 57
  • 102