Corrupted Heap, trying to make custom vector

[Conny14156]

Solved:
I did not have a operator= for my customvector, because when I copied the Person (which at than did not have a copy constructor) it copied the vector adress, so it got double deleted, once from the original Person, and than again from the copied Person.

Person::Person(Person& a)
{
	id = a.id;
	gender = a.gender;
	name = a.gender;
	hobbies = a.hobbies;
}

	void operator=(CustomVector& a)
	{
		if(currentlyList)
		{
			delete[] currentlyList;
			currentlyList = 0;
		}
		currentlyList = new TemplateClass[a.max];
		std::cout << "\nNew from Opeartor= : " << currentlyList;
		current = a.current;
		max = a.max;
		size = a.size;
		for(int i = 0; i < a.Size(); i++)
			currentlyList[i] = a[i];
		
	};

Hi,
Am trying to make a copy of the std::vector (A lesser version). I am doing this as a educational problem. But I got into some kind of problem. I get some heap corruption and sometimes breakpoint throw. with google, its seems like I sometimes try to double delete ( _Block_Type_Is_Valid ), but also sometimes writing to (CRT detected that application wrote to memory after end of heap buffer.).

But the latest problem is just the double delete, am not sure where I double delete.

template<class TemplateClass>
class CustomVector
{
	TemplateClass* currentlyList;
public:
	int max;
	int current; 
	CustomVector()
	{
		current = -1;
		size = 0;
		max = 5;
		currentlyList = new TemplateClass[max + 1];

	}

	CustomVector(CustomVector& a)//Tried to do a deep copy
	{
		currentlyList = new TemplateClass[a.max + 2];
		current = a.current;
		max = a.max;
		size = a.size;
		for(int i = 0; i < a.Size(); i++)
		{
			TemplateClass* tempABBA = new TemplateClass(a[i]);
			currentlyList[i] = *tempABBA;
		}
	}
	~CustomVector()
	{
		delete[] currentlyList;//Deletes when it dies
		currentlyList = 0;
	}
	void add(TemplateClass  a)//Had 
	{
		push_front(a);
	}
	TemplateClass* at(int i)
	{
		if(i <= current)
		{
			return &currentlyList[i];
		}
		return 0;
	}
	TemplateClass& operator[](int i)
	{
			return currentlyList[i];
	}
	
	int Size()
	{
		return current + 1;
	}
	void push_front(TemplateClass a)
	{
		if(current + 2 < max)
		{
			for(int i = Size(); i > 0; i--)
				currentlyList[i] = currentlyList[i-1];
			currentlyList[0] = a;
			current++;
		}
		else
		{
			TemplateClass* temp;
			max += Size() + 10;
			temp = new TemplateClass[max+1];
			for(int i = 0; i < Size(); i++)
				temp[i + 1] = currentlyList[i];
				
			temp[0] = a;
			current++;
			if(currentlyList)
			{
				if(Size())
				{
					delete[] currentlyList;//Problem here 
					currentlyList = 0;
				}
			}
			currentlyList = temp;
		}
	}
	

[dhayden]

Lines 13, 19 etc.: Why do you have +1 and +2? I think you will find it easier if current is the current number of items in the vector and max is the number than can go in without resizing it (aka the capacity).

Lines 25 & 26, you don't need to allocate new items on the heap. In fact you'll just leak them anyway. These lines should be
currentlyList[i] = a[i];

Lines 34-37: Why does add() put the item on the front? If you look at std::vector, you'll notice that you can only push items on the back. This is because adding items to the front is very expensive since you have to move all the existing items.

Line 38: What if i is negative? I should probably be a size_t instead since size_t is unsigned.

Line 40: Okay, it looks like current is the max current index

Line 55: Pass a by reference to avoid copying it: push_front(TemplateClass &a)
Line 57: current+2? so max is...? Hmm...

Line 43: add an else here and print out a message if the index is out of bounds. You can remove the message after you have the code working. I believe std::vector throws an exception.

Line 73: You increment the size here but I think you mean to check the old size at line 76. So you're always executing the delete at line 78. Actually this is a good thing because the it doesn't matter what the current size is, you want to delete currentlyList if it isn't NULL. This is an example of code that works by accident, or what I like to call "cooperating bugs." Two bugs offset each other to create working code. If you fix one bug, you will expose the other one.

I think what you want to do here is move line 73 to after line 82: adjust current and currentlyList at the same time. Then delete if currentlyList is non-null:

if (currentlyList) {
    delete[] currentlyList;
}
currentlyList = temp;
current++;

I don't see a double delete here. Maybe the code that calls it is doing something funny. There are other things that should be changed, but they are slightly more advanced. Let's get the code working first and then clean it up.


Line 76:

[Conny14156]

When I first got the memory/heap leak, I read somewhere I had to include some memory for NULL, which was the reason I had +1 from the start. And I guess it "worked" (But probably not for the reason I believed to be). Because after awhile I started to get the same bug again, and silly me just tried +2 instead for some reason(Guess that what happened when I code at 2am ish). And after a lot of changes I forgot to changes it, and the reason why add() just using push_front was because it had different code there that I commented out, and I didn't want to spam the code section with tons of commented code.
I seems to double delete at destructor and from push_front. My noob guess is that I done something funny when I try to copy a vector to another.
http://imgur.com/sxNGqej


This is the "main" code

	MatchMaking mm;
	Person* person = new Person[20];//Same result
	Person person[20]; //Same result

	for(int i = 0; i < 20; i++)//Crashes at i == 6
	{
		mm.males.push_front(person[i]);
                mm.males.push_front(i);//this worked(after I made a customvector<int> instead of <Person>
	}

Here the "updated" class code
P.S The function add() is currently not in use, the same is said about remove

template<class TemplateClass>
class CustomVector
{
	TemplateClass* currentlyList;
	int size;
public:
	int max;
	int current;
	CustomVector()
	{
		current = -1;
		size = 0;
		max = 5;
		currentlyList = new TemplateClass[max];
		std::cout << "\nNew from Constructor: " << currentlyList;

	}
	CustomVector(CustomVector& a)
	{
		currentlyList = new TemplateClass[a.max];
		std::cout << "\nNew from Copy Constructor: " << currentlyList;
		current = a.current;
		max = a.max;
		size = a.size;
		for(int i = 0; i < a.Size(); i++)
		{
			currentlyList[i] = a[i];
		}
	}
	~CustomVector()
	{
		if(currentlyList)
		{
			std::cout << "\nDeleting from Destructor: " << currentlyList;
			delete[] currentlyList;
			currentlyList = 0;
		}
	}
	TemplateClass* at(size_t i)
	{
		if(i <= current)
		{
			return &currentlyList[i];
		}
		else
		{
			std::cout << "Out of Bounds";
			return 0;
		}
	}
	TemplateClass& operator[](size_t i)
	{
		return currentlyList[i];
	}
	int Size()
	{
		return current + 1;
	}
	void push_front(TemplateClass a)
	{
		if(Size() < max)
		{
			for(int i = Size(); i > 0; i--)
				currentlyList[i] = currentlyList[i-1];
			currentlyList[0] = a;
			current++;
		}
		else
		{
			TemplateClass* temp;
			max += current + 10;
			temp = new TemplateClass[max + 1];
			std::cout << "\nNew from Push_Front: " << temp;

			for(int i = 0; i < Size(); i++)
				temp[i + 1] = currentlyList[i];
			temp[0] = a;
			if(currentlyList)
			{
				std::cout << "\nDeleting with push_front: " << currentlyList;
				delete[] currentlyList;
				currentlyList = 0;
			}
			currentlyList = temp;
			current++;
		}
	}
	void add(TemplateClass  a)
	{
		if(Size()  < max)
		{
			currentlyList[current + 1] = a;
			current++;
		}
		else
		{
			TemplateClass* temp;
			max += Size() + 10;
			temp = new TemplateClass[max];
			std::cout << "\nNew from Add: " << temp;

			for(size_t i = 0; i < Size(); i++)
				temp[i] = currentlyList[i];

			if(currentlyList)
			{
				std::cout << "\nDeleting from add: " << currentlyList;
				delete[] currentlyList;
				currentlyList = 0;
			}
			currentlyList = temp;
			current++;
			currentlyList[current] = a;
		}
	}
};

The person class

class Person
{
public:
	int id;
	Person();
	CustomVector<int> hobbies;//This seems to be the problem, when I remove this I dont get the double delete
	std::string name;
	bool gender;
};

Person::Person()
{
	std::random_device random;
	std::srand(time(NULL));
	this->name = "Alex";
	gender = random() % 2;
	int maxHobby = random() % 6;
	int tempHobbyID = 0;
	bool dub = 0;
	id = random();
	for(int i = 0; i < maxHobby; i++)
	{
		tempHobbyID = random() % 6;
		for(int j = 0; j < this->hobbies.Size(); j++)
		{
			if(tempHobbyID == hobbies[j])
			{
				dub = true;
				break;
			}
		}
		if(!dub)
			hobbies.push_front(tempHobbyID);
	}
}

[dhayden]

Ah. You need an assignment operator in CustomVector<>. When you assign a Person instance, it will call the assignment operator of CustomVector<int>hobbies. Since none is defined, you get the default which just copies the members, including currentlyList.

[dhayden]

When I first got the memory/heap leak, I read somewhere I had to include some memory for NULL

You need space for a NULL when allocating room for a NULL-terminated string:

const char *myString = "hello world";
const char *dup = new char[strlen(myString)+1]; // room for null terminator
strcpy(dup, myString);

The point here is that strlen(myString) returns 11 but you need 12 characters to store the string due to the terminating null byte.

This is just one reason why it's a good idea to use std::string instead of null-terminated strings.

[megatron 0]

Great effort with your custom vector!

You may want to check out std::allocator from the <memory> header along with functions such as std::uninitialized_fill and std::uninitialized_copy instead of using those new and deletes for performance reasons :)

Still great work!