A heap has been corrupted when trying to run code

Question

When ever I run my program it breaks and has the error "A heap has been corrupted" when debugging the program, it actually will go through the whole thing just fine and break on the system("PAUSE") which seems like an odd place to have an error. I am clueless where the issue is. The program worked just fine, until I added the operator overload for + and OS

Below is my code:

MAIN.CPP

#include "stdafx.h"
#include "vector.h"

// the printV function
// used to test the copy constructor
// parameter: a MyVector object

void printV(Vector);


int main()
{
    cout << "\nCreating a vector Sam of size 4.";
    Vector sam(4);

    cout << "\nPush 12 values into the vector.";
    for (int i = 0; i < 12; i++)
        sam.push_back(i);

    cout << "\nHere is sam: ";
    cout << sam;
    cout << "\n---------------\n";

    cout << "\nCreating a vector Joe of size 4.";
    Vector joe(4);
    cout << "\nPush 6 values into the vector.";
    for (int i = 0; i < 6; i++)
        joe.push_back(i * 3);

    cout << "\nHere is joe: ";
    cout << joe;
    cout << "\n---------------\n";

    cout << "\nTest the overloaded assignment operator \"joe = sam\": ";
    joe = sam;

    cout << "\nHere is sam: ";
    cout << sam;
    cout << "\n---------------\n";

    cout << "\nHere is joe: ";
    cout << joe;
    cout << "\n---------------\n";

    // pass a copy of sam by value
    printV(sam);

    system("PAUSE");
    return 0;
}

void printV(Vector v)
{
    cout << "\n--------------------\n";
    cout << "Printing a copy of a vector\n";
    cout << v;
}

VECTOR.H

#pragma once
#include <iostream>
#include "stdafx.h"

using namespace std;

class Vector
{

private:
    int vectorSize;
    int vectorCapacity;
    int *vectorArray;

public:

    //A default constructor that creates an vector with a default capacity of 2
    Vector();

    //A parameterized constructor that creates a vector of capacity n
    Vector(int n);

    // A function, size(), that returns the size of your vector.
    int size();

    // A function, capacity(), that returns the capacity of the vector.
    int capacity();

    // A function, clear(), that deletes all of the elements from the vector and resets its size to zero and its capacity to two.
    void clear();

    // A function push_back(int n) that adds the integer value n to the end of the vector.If the vector is not large enough to hold this additional value, you must make the vector grow.Your grow algorithm should double the current capacity of the vector.Don't forget to consider the case where the initial capacity of the vector is zero.
    void push_back(int n);

    // A function at(int n) that returns the value of the element at position n in the vector.If the index n is greater than the size() of the vector, this function should throw an exception.
    int at(int n);

    friend ostream& operator<<(ostream& os, Vector vt);

    Vector operator=(Vector&);

VECTOR.CPP

#include "stdafx.h"
#include "vector.h"


Vector::Vector()
{
    vectorSize = 0;
    vectorCapacity = 0;
    vectorArray = 0;

}
// Create new array with given capacity
Vector::Vector(int n) 
{
    vectorCapacity = n;

    vectorArray = new int[vectorCapacity];

}
//Return array size
int Vector::size() 
{
    return vectorSize;
}

// Return array capacity
int Vector::capacity()
{
    return vectorCapacity;
}

// clear array values
void Vector::clear()
{
    for (int i = 0; i < sizeof(vectorArray); i++)
    {
        vectorArray[i] = '\0';
    }

    vectorSize = 0;
    vectorCapacity = 2;
}


// Add number to array and double array size if needed

void Vector::push_back(int n)
{
    int test = 100;
    if (vectorCapacity > vectorSize)
    {
        vectorArray[vectorSize] = n;
        vectorSize++;

    }
    else {

        if (vectorCapacity == 0) {
            vectorArray = new int[4];
            vectorArray[0] = n;
            vectorCapacity = 4;
            vectorSize++;
        }
        else {

            int newCapacity = vectorCapacity * 2;

            // Dynamically allocate a new array of integers what is somewhat larger than the existing array.An algorithm that is often used is to double the size of the array.

            int *tempArray = new int[newCapacity];

            // Change capacity to be the capacity of the new array.

            vectorCapacity = newCapacity;

            // Copy all of the numbers from the first array into the second, in sequence.

            for (int i = 0; i < Vector::size(); i++)
            {
                tempArray[i] = vectorArray[i];
            }

            delete[] vectorArray;
            vectorArray = new int[newCapacity];

            for (int i = 0; i < Vector::size(); i++)
            {
                vectorArray[i] = tempArray[i];
            }

            delete[] tempArray;

            // Add the new element at the next open slot in the new array.

            vectorArray[vectorSize] = n;

            // Increment the size;

            vectorSize++;

        }
    }
}


// Return Value and given point in array

int Vector::at(int n)
{
    return vectorArray[n];
}

// Cout Vector
ostream& operator<<(ostream& os, Vector vt)
{
    int size = vt.size();

    for (int i = 0; i < size; i++) {

        os << "index " << i << " is " << vt.at(i) << endl;

    }

    return os;
}

// Set one vector to equil another
Vector Vector::operator=(Vector& right) {

    // Clear array on left
    for (int i = 0; i < sizeof(vectorArray); i++)
    {
        vectorArray[i] = '\0';
    }

    vectorSize = right.size();
    vectorCapacity = right.size() * 2;

    // Assign values from left to right
    for (int i = 0; i < vectorSize; i++)
    {
        vectorArray[i] = right.at(i);
    }

    return vectorArray[0];

}

Answer 1

The problem is operator=()

Why ?

You start with sam having a capacity of 4. You push back 12 items in it. When you reach the 5th element, the capacity is doubled from 4 to 8. Then you then reach the 9th element, the capacity is increased to 24.

You then have joe with an initial capacity of 4. You push back 6 items in it. When you reach the 5th element, its capacity is increased to 8.

When you then do joe = sam, your operator overwrites joe's size and capacity but without verifying that the capacity matches, and without allocating missing capacity. As you then try to copy 12 elements in a vector having in reality only a capacity of 8, you do some collateral damage in memory and corrupt the heap.

Solution

Do not overwrite blindly the capacity. Keep the capacity if it's sufficient. If not, align the capacity and reallocate.

// Set one vector to equal another
Vector Vector::operator=(Vector& right) {

    //...    

    if (vectorCapacity < right.vectorCapacity) {
        delete[] vectorArray;   // assuming pointer is either nullptr or valid array
        vectorArray = new int[right.vectorCapacity];
        vectorCapacity = right.vectorCapacity;   
    }
    vectorSize = right.size();
  
    // Assign values from left to right
    //...

    return *this;
}

Note that it would be better to return the vector by reference !

Answer 2

There are lots of errors, but the one that causes the described symptoms is the operator= never allocates a new array of int for vectorArray

Each use of sizeof(vectorArray) is also wrong. That is just the size of a pointer, not the allocation of the area pointed to.

Each place that does vectorArray[i] = '\0'; is at best pointless, and whatever was intended, that is the wrong way to do it. Enough so I can't even guess the intent.

In the clear function the only necessary step was vectorSize = 0; The rest was at best pointless. Setting capacity to 2 there is bizarre, though it does no major harm.

operator= ought to have return type Vector& and not Vector and should return *this not construct a Vector whose capacity is a value from the old one. In general, almost any operator= member of a class should return *this. Exceptions to that rule are way beyond the level where you are currently trying to learn.

Answer 3

Given all of the answers so far, the other issue is that you failed to implement a user-defined copy constructor:

Vector(const Vector& n);

This function has to be implemented correctly since you have functions returning Vector by value. Since you didn't implement it, copying will not work correctly.

Second issue is that you should be returning the Vector by reference, not by value in the operator= function.

My first suggestion is to take whatever code you have now in your operator= and do the work of the "real" copying in the copy constructor. Here is a streamlined version of what the copy constructor should look like:

#include <algorithm>
//..
Vector::Vector(const Vector& rhs) : vectorCapacity(rhs.vectorCapacity), 
                                    vectorArray(new int[rhs.vectorCapacity]), 
                                    vectorSize(rhs.size())
{
    std::copy(rhs.vectorArray, rhs.vectorArray + rhs.vectorCapacity, vectorArray);
}

Note the usage of the member initialization list, and the call to the function std::copy (you could also have written a loop, but just to show you that there are functions that do the copy without hand-written loops).

The second thing is that your destructor should simply do this:

Vector::~Vector()
{  delete [] vectorArray; }

Then operator= becomes very simple using copy / swap.

#include <algorithm>
//...
Vector& operator=(const Vector& v)
{
   Vector temp = v;
   swap(this.vectorCapacity, temp.vectorCapacity);
   swap(this.vectorArray, temp.vectorArray);
   swap(this.vectorSize, temp.vectorSize);
   return *this;
}

This works only if the copy constructor and destructor work correctly, since operator= takes advantage of these functions.

Read more about the Rule of 3 and the copy / swap idiom.