Calling delete[] breaks my C++ program

Question

Updated: Thanks for the quick replies. It seems that I have posted an older version of the code. Everything remains the same except for the parameterized constructor. There are a few flaws in the code as you can see, but bare in mind that I am not fully completed with this. Currently I am more worried about the array since this is a new concept introduced yesterday. I have tried several different things and researched this for hours. Most of the responses say to just use the vector class but this is for homework which helps us understand memory allocation and dynamic arrays. Currently this is my .cpp and .h file that is giving me problems. Every time the delete (or the clear function) operation is triggered an error occurs that states blahblah.exe has triggered a break point.

MyVector.h

#pragma once

class MyVector
{
private:
    int arraySize; 
    int arrayCapacity;
    int* theData;
    void grow();

public:
    MyVector();
    MyVector(int n);
    int size() const;
    int capacity() const;
    void clear();
    void push_back(int n);
    int& at(int n);
    ~MyVector();
};

MyVector.cpp

   #include "MyVector.h"
    #include <iostream>
    using namespace std;

    MyVector::MyVector()
    {
    arraySize = 0;
    arrayCapacity = 0; 
    theData = new int[0];
    }
    MyVector::MyVector(int capacityIn)
    {
    theData = new int [capacityIn];
    arraySize = 0;
    arrayCapacity = 0;
    }
    int MyVector::size() const
    {
    return arraySize;
    }
    int MyVector::capacity() const
    {
    return arrayCapacity;
    }
    void MyVector::clear()
    {
        delete [] theData;
        theData = nullptr;
    }
    void MyVector::push_back(int n)
    {
        if (arrayCapacity==0)
        {
        arrayCapacity++;
        MyVector(arrayCapacity);
    }
    if (arraySize == arrayCapacity)
    {
        grow();
        MyVector(arrayCapacity);
    }
    theData[arraySize] = n;
    arraySize++;
}
int& MyVector::at(int index)
{
    if (index >= 0 && index<arraySize)
    {
        return (theData[index]);
    }
    else
    {
        throw index;
    }
}
void MyVector::grow()
{
    arrayCapacity = arrayCapacity + arrayCapacity;
}

MyVector::~MyVector()
{
    if (theData != nullptr)
    {
        clear();
    }
}

Answer 1

You have a number of problems I can see already:

1. You should not be calling the constructor explicitly like this. It doesn't do what you think. Put allocations in a real member function.
2. Every time you "grow" your array, you just allocate new, causing a leak because the previous pointer wasn't deleted.
3. The main issue you're asking about: You're not even storing the pointer you allocate in the constructor. Change int* theData = new int [capacityIn]; to theData = new int [capacityIn];. You're doing it correctly in your first constructor for example.
4. You are not initializing arraySize or arrayCapacity in your second constructor (MyVector(int)).

Nitpicks:

1. You don't need to check for nullptr before deleting.
2. There's no need to new int[0]. You should never access the memory, so just initialize it to nullptr.

Answer 2

Here:

MyVector::MyVector(int capacityIn)
{
  int* theData = new int [capacityIn];
}

You declare a local pointer theData that shadows the theData data member of the current object, leaving it uninitialized. From there, all bets are off on using it, and it is pure happenstance that it is the delete[] where your program finally crashes. Write

MyVector::MyVector(int capacityIn)
{
  theData = new int [capacityIn];
}

instead, and arraySize and arrayCapacity will have to be initialized as well.

Apart from that, you'll run into the problem that

MyVector(arrayCapacity);

in the push_back function allocates a temporary object of type MyVector that is (almost) immediately destroyed again; it does not change the current object.

Answer 3

This is causing you the problem you cannot solve:

int* theData = new int [capacityIn];

You are saying: "Please define an integer pointer..." and so on. The member variables are different than the variables you are defining inside you methods; it makes no sense to do that. Instead, you should have done that:

theData = new int [capacityIn];

or, just for educational purposes (and in order for you to understand we are assigning to a member variable):

this->theData = new int [capacityIn];

Also (some points may not be that important, but I would like to point them out since you are a new guy):

1. Please set your other variables in the second constructor, it seems you have forgotten to do so.

   MyVector::MyVector(int capacityIn)
   {
       theData = new int [capacityIn];
       arraySize = capacityIn;
       arrayCapacity = capacityIn;
   }
   

2. You should have meaningful names, theData is something you should definitely avoid. http://petdance.com/2012/04/the-worlds-two-worst-variable-names/

3. Your methods should start with a verb, e.g. getDimension() getCapacity() and so on.
4. Constructors are... constructors, which means they are meant to be called whenever an item is constructed or created. You definitely should not explicitely call a constructor in any way.
5. This is a basic rule: whenever you access the memory with the new command, you should call delete somewhere after some time. Your code has severe memory leaks.

I might have been able to help you more, but I do not exactly understand what you are trying to accomplish in some methods, namely in grow() and pushBack().

Answer 4

It's not good style to initialize variables via assignment statements inside the constructor body. Instead, variables should be initialized before the constructor body is entered.

You can either do this by using the member initializer list, or by supplying default values in the class definition. The latter seems easiest here:

class MyVector
{
private:
    int arraySize = 0;
    int arrayCapacity = 0;
    int* theData = nullptr;
    // ....

In the constructor then instead of duplicating code you should leverage existing functions to do the same thing:

MyVector::MyVector(int capacityIn)
{
     // call function which increases capacity to match capacityIn.
}

Currently you don't actually have a function to increase the capacity so you will need to add one. (The grow() function makes the int variable increase but you don't allocate any more memory so this just results in your code writing past the end of the space that is actually allocated).

It might look like:

void grow( int newCapacity )
{
    if ( newCapacity < arrayCapacity )
        return;   // we do not need to shrink

    int *newData = new int [new_capacity];
    if ( arraySize > 0 )
        std::copy(arrayData, arrayData + arraySize, newData);
    delete[] arrayData;
    newData = arrayData;
}

Then you can modify grow() (if you still want to keep that), and push_back() to also call this function.

---

Note that the clear() function simply needs to do arraySize = 0; . It doesn't need to free any memory; you can leave that capacity available for future use.