heap and class destructor

Question

I have a problem with my code. I get an error of BLOCK_TYPE_IS_VALID... I know there is problem with new and delete, but I cannot find it. I have a myString class with these functions:

    //constructors and destructors
myString::myString() {
    this->string_ = NULL;
    this->length = 0;

    cout << "created " << "NULL" << endl;
}

myString::myString(char a[]) {
    int i;
    for (i=0; a[i]!=NULL; i++);
    this->length = i;

    int j=0;
    while(pow(2,j)<i)
        j++;

    this->string_ = new char [(int)pow(2,j)];
    for (int i=0; i<this->length; i++)
        this->string_[i] = a[i];

    cout << "created " << this->string_ << endl;
}

myString::~myString() {
    cout << "deleteing " << this->string_ << endl;
    if (this->string_ != NULL)
        delete [] this->string_;
}

and when I run this

myString a("aaa");
myString b("bbbb");
myString c;
c = a + b;
cout << c.get_lenght() << endl;
cout << c.get_string() << endl;

I get the error in line "c = a+b" and then program stops.

You need to define the `operator+` in your class so that program knows how to add the strings. — Caesar, Apr 01 '13 at 17:38
Have you overload operator `+` & operator `=`? Can you show that code? — user93353, Apr 01 '13 at 17:40
Based on the code I see, I hope you have defined the copy constructor and assignment operator. Can you show that code? — Drew Dormann, Apr 01 '13 at 17:42
Avoid calls to pow with `int pow2 = 1; while (pow2 < i) pow2 *= 2;` You might also want to keep this number in your class as a capacity variable. — emsr, Apr 01 '13 at 17:57

score 3 · Answer 1 · answered Apr 01 '13 at 17:55

You need to define a copy constructor and assignment operator for your class.

myString::myString( const myString& );
myString& operator=( const myString& );

Otherwise, you are violating the rule of three.

This code...

c = a + b;

may produce a temporary myString holding the value a + b.

The default generated copy and assignment implementations will give c the same string_ pointer that the temporary has.

And when the destructor for either of those strings is run, the other string will have a dangling pointer.

Coincidentally, this code:

if (this->string_ != NULL)
    delete [] this->string_;

Will never act differently than simply:

delete [] this->string_;

Thank you, I was very confused. Now I understand what happens. — Ramyad, Apr 01 '13 at 18:50

score 1 · Accepted Answer · edited May 23 '17 at 12:13

You haven't shown the class definition, but I'm guessing that you haven't followed the Rule of Three.

Without a correctly implemented copy constructor and copy-assignment operator, it is not possible to copy objects safely. The default implementation will simply copy the pointer (and other member variables), leaving both copies to delete the same block of memory in their destructors.

The simplest solution is to use a class designed to manage memory for you. std::string or std::vector<char> would be ideal here.

Assuming you have a good reason for managing the memory yourself, you will need something like:

// Copy constructor
myString(myString const & other) :
    string_(new char[other.length]),
    length(other.length)
{
    std::copy(other.string_, other.string_+length, string_);
}

// Simple assignment operator
// For bonus points (and a strong exception guarantee), use the copy-and-swap idiom instead
myString & operator=(myString const & other) {
    if (this != &other) {
        delete [] string_; // No need to check for NULL (here or in the destructor)
        string_ = new char[other.length];
        length = other.length;
        std::copy(other.string_, other.string_+length, string_);
    }
    return *this;
}

In C++11, for even more bonus points, consider also providing a move constructor and assignment operator. These just need to modify the pointers, so will be much more efficient than copying.

heap and class destructor

2 Answers2