CODEWITHSUNDEEP
c++arraysstringdelete-operator
Dalibor
Dalibor

Reputation: 11

C++ delete[] crashes

I'm writing a C++ program, that stores strings in a string array, when the array is full I resize the array to make space for more items using the code below. But sometimes (not always) it crashes at the "delete[] temp;" line and I don't know why and how to fix it. Please, help.

I have searched a lot but could not find an answer anywhere. When I debug it says "invalid pointer" but how can it be invalid when I stored data there before and did not free it yet?

This is my code:

if(item_cnt >= (arr_size - 1))
{
    int oldsize = arr_size;
    string * temp;
    arr_size *= 2;
    temp = arr;
    arr = new string [arr_size];
    memcpy(arr, temp, oldsize * sizeof(temp));
    delete[] temp;
}

Upvotes: 0

Views: 2061

Answers (4)

Mario The Spoon
Mario The Spoon

Reputation: 4869

Mixing old style and new style memory operations is always a bad idea... here you use memcpy and new/ delete. be aware that delete[] also calls the dtor for each element of the array...

Upvotes: 0

Roddy
Roddy

Reputation: 68074

The memcpy is at the root of your problem. everyone's said "don't use it", but let me explain exactly why it's a terminally bad idea.

First off, what is a c++ string, and how does it do its magic? It's basically a variable-length array of characters, and it achieves this feat by holding a pointer within each string object that points to the memory allocated to hold those characters. As the string grows or shrinks, that memory gets reallocated. Copy strings properly involves making a 'deep copy' of the contents.

Now, to your code:

arr = new string [arr_size];

This creates an array of empty string objects. Because they're empty, the internal pointers are typically null.

memcpy(arr, temp, oldsize * sizeof(temp));

Here, bad things happen. This isn't actually creating copies of the original strings, it's just overwriting the internal representation. So both the old and the new strings 'point' to the same character data. Now to really screw things up, this happens:

delete[] temp;

We delete thew old strings, but this also frees up the character memory that they were using. So our 'new' copies of these strings are pointing at memory that's actually been freed. We now have a car-crash waiting to happen : The character data could be re-used for anything, and when we try and delete the strings again, the operating system will hopefully spot that you're trying to free memory that hasn't been allocated.

Upvotes: 1

piokuc
piokuc

Reputation: 26204

Your array should be really 

vector<string>

This is a recommended way of implementing arrays of dynamic size. By using vector you avoid necessity to reallocate/copy stuff manually and avoid problems like the one you have altogether.

Upvotes: 0

Pepe
Pepe

Reputation: 6480

Unless you absolutely have to stick with your current approach, I would recommend using a vector to hold your strings. It will manage all the memory for you.

Here's an example:

#include <vector>
#include <string>
int main()
{
   std::vector<std::string> arrayOfStrings;

   arrayOfStrings.push_back("Hello World!"); // To Add Items
   string value = arrayOfString.at(<some index>); // To Retrieve an Item you can also use the [] operator instead of the at method

   return 0;
}

Upvotes: 5

Related Questions