1

I am using this algorithm for dynamically increasing the size of an array:

void resize(int* oldArray, int* capacity) {
    *capacity = *capacity * 2;
    int* new_array = new int[*capacity];
    for (int i = 0; i < *capacity/2; i++) {
        new_array[i] = oldArray[i];
    }
    delete[] oldArray;
    oldArray = new_array;
}

However, when I build and run this algorithm I get the following error:

r3(21751,0x7fff8d76e340) malloc: * error for object 0x7f8ca5c026c0: pointer being freed was not allocated * set a breakpoint in malloc_error_break to debug Abort trap: 6

Also, this is the main function I am using:

int main(int argc, char* argv[]) {
    if (argc != 2) {
        return -1;
    }

    std::ifstream inFile(argv[1]);
    int capacity = 10;
    int* num_array = new int[capacity];
    int num_elements = 0;

    if (inFile.is_open()) {
        std::string line;
        while (!inFile.eof()) {
            getline(inFile,line);
            if (num_elements == capacity) {
                resize(num_array,&capacity);
            }
            int num = stoi(line);
            num_array[num_elements++] = num;
        }

        for (int i = 0; i < num_elements; i++) {
            std::cout<<num_array[i]<<std::endl;
        }
    }
}

Can anyone please tell me why this is happening? Thank you!

max66
  • 65,235
  • 10
  • 71
  • 111
wfulton
  • 21
  • 3

3 Answers3

3

It means you're a "C+ programmer", that strange breed who appears to have not fully made the transition away from C :-)

Things such as using malloc/free (and even new/delete in these days of smart pointers), raw arrays rather than the standard collections, rolling your own code for things found in <algorithms>, using printf rather than streams, and so on, these are the things that you should generally steer clear of.

I'd suggest you look into using std::vector if you want resizable arrays, it's part of the language itself and has been well and truly tested (a).

All that gumpf about resizing the array and adding your new element can then be replaced with a simple:

myVector.push_back(myElement);

(a) Yes, I'm well aware that it doesn't answer your specific question but, like writing an accounting package in assembler, developing your new operating system in object-oriented COBOL, or doing anything in Pascal :-), sometimes the right answer is "don't do that".

If you do insist on being a "C+ programmer", you should at least understand that everything you want to change in a function and have the effect returned to the caller, should be passed by reference. In this case, that's both your pointer and the capacity variable (there is no need to use C's hideous pointer-based pass-by-reference trickery in C++):

#include <algorithm>
:
void resize(int *&oldArray, int &capacity) {
    capacity *= 2;
    int newArray = new int[capacity];
    std::copy(oldArray, oldArray + capacity / 2, newArray);
    delete[] oldArray;
    oldArray = newArray;
}

You'll see a few improvements there, specifically:

  • proper pass-by-reference for the variables.
  • using std::copy to copy elements, rather than coding your own loop.
  • consistent naming (not strictly necessary but useful to satisfy my CDO (b)).

(b) OCD but in the correct damn order :-)

paxdiablo
  • 854,327
  • 234
  • 1,573
  • 1,953
2

You need a &

// ...............V
void resize(int * & oldArray, int* capacity) {
*capacity = *capacity * 2;
int* new_array = new int[*capacity];
for (int i = 0; i < *capacity/2; i++) {
    new_array[i] = oldArray[i];
}
delete[] oldArray;
oldArray = new_array;
}

If you pass a pointer to resize(), without a & to pass it by reference, you pass it by copy. So you can change the values of the pointed integers but not the value of pointer itself.

With

int* num_array = new int[capacity];

you dynamically allocate an area, the first one.

With the first call to

resize(num_array,&capacity);

the area is destroied (inside resize()) and a second area is allocated but lost because, in main(), num_array maintain the value of the first area.

With the second call to

resize(num_array,&capacity);

the first allocated area is deleted for the second time.

Crash!

Passing oldArray as reference, the value of the new allocated area is assigned to the num_array variable in main().

max66
  • 65,235
  • 10
  • 71
  • 111
  • `capacity` should also be a reference, there is zero need to use the painful C method of emulating pass-by-reference in C++ :-) – paxdiablo Jan 31 '18 at 03:05
  • Obviously you're right. I was undecided whether to write it in my answer. There were a lot of things to write about the code in the question (and you wrote a lot of things :-) ) but I was afraid to discourage the OP and I tried to focus on the point that caused the error. – max66 Jan 31 '18 at 12:31
1

You should pass a reference of pointer num_array in case you want to allocate new memory for this pointer, which means

you should change void resize(int* oldArray, int* capacity) to void resize(int* & oldArray, int* capacity).

The reason behind this is that, when you calling function resize with passing argument num_array, you just created a new pointer type variable called oldArray and initialized it with the same value of num_array in the function body.

So when you new a memory, you just allocated memory for this new variable oldArray but not for the argument num_array. But when you calling delete[] oldArray, it's actually freed the memory you had allocated for num_array since oldArray has the same value with num_array here.

To get rid of this problem, you should just pass the reference of num_array.

Francis
  • 737
  • 1
  • 7
  • 21
  • 2
    Deleting a null pointer is well-defined. No need to check it first. – Retired Ninja Jan 31 '18 at 03:44
  • I have checked the reference, you are right. `delete` has already done this checking for me. Thanks for pointing out my incorrectness. I have deleted this part.@RetiredNinja – Francis Jan 31 '18 at 04:12