Why does my code crash?

[LGOOZ1]

I'm kind of new in C++, only about a week in. I'm using VS10 for writing my exercises from a C++ book. I have previous C experience but does not seem to help too much in understanding the run-time errors I'm getting.

I wrote a class called student that has constructors, destructor and some other pretty trivial member methods (not to say functions).

Among other things, I'm overwriting the + operator in such a way:

student student::operator+( const student &vec )
{
	student temp( *this ); // copy constructor

	if (age > vec.age)
		temp.age = age;
	else
		temp.age = vec.age;

	if (strlen(name) > strlen(vec.name))
	{
		temp.name = new char[strlen(name)+1];
		strcpy(temp.name,name);
	}
	else
	{
		temp.name = new char[strlen(vec.name)+1];
		strcpy(temp.name,vec.name);
	}

	if (n > vec.n)
	{
		temp.n = n;
		temp.grades = new int[temp.n];
		for(int i=0;i<n;i++)
			temp.grades[i] = grades[i];
		for(int i=0;i<vec.n;i++)
			temp.grades[i]+= vec.grades[i];
	}
	else
	{
		temp.n = vec.n;
		temp.grades = new int[temp.n];
		for(int i=0;i<vec.n;i++)
			temp.grades[i] = vec.grades[i];
		for(int i=0;i<n;i++)
			temp.grades[i]+= grades[i];
	}

	return temp;
}

And here is my destructor:

student::~student()
{
   delete[] name, grades;
   name = NULL;
   grades = NULL;
}

The reason I'm putting this here is because the error seems to be popping when the return command in the operator overwrite is executed and this calls the constructor and destructor - I don't know why exactly.

So the program runs OK if I ignore the error. I don't get a very specific error just that some violation in the heap had occurred. If someone will further direct me I might be able to pull out a better error report.

[Cubbi]

"delete[] name, grades;" doesn't do what you think it does: it is a comma operator separating the expressions "delete[] name" (which, when executed, frees the memory pointed to by name), and the expression "grades", which, when executed, does nothing at all.

But that's just a memory leak, not a runtime crash. Seeing as you had to define an assignment operator and a destructor, the first question is: how did you define the copy constructor? and, seeing as you're relying on vec.name to be valid C string, how did you define other constructors?

If helps to post a complete, compilable, minimal example, when you have a problem you want someone else to debug.

[Disch]

delete[] name,grades ;

This doesn't do what you think. It does not delete both name and grades... it only deletes one of them (I forget which ... the comma operator is weird).

In general... the comma operator doesn't do what you think it does. Avoid it.

A heap error sounds like you are trying to dereference a pointer after it's deleted... or are deleting something that had already been deleted. It's hard to say without being able to repro the error (if you run from the debugger, it should tell you exactly what line it's happening on)


So long story short... you are screwing up your memory management somehow.

But you shouldn't be doing manual memory management anyway. C++ offers containers for all this stuff.


Instead of using a char*, you should be using a string.
Instead of using a int*, you should be using a vector<int>.

If you do that, you do not need to overload the = operator, or destructor... or have to do any memory management. All of it will be done automatically.




Can you post the student class in full? That would help me spot the error.

[LGOOZ1]

Thanks for the quick reply.

Here is the error I am getting: http://s13.postimg.org/msp1r1vaf/err.png

[LGOOZ1]

Thanks for the quick reply.

Here is the exact error I am getting. I am not sure what is the official name for it.
http://s13.postimg.org/msp1r1vaf/err.png

I understand what you say so I changed the destructor like so:

student::~student()
{
	delete[] name;
	delete[] grades;
	name = NULL;
	grades = NULL;
}

Here is the copy constructor:

student::student( const student & a )
{
	n = a.n;
	age = a.age;
	name = new char[sizeof(a.name)+1];
	strcpy(name,a.name);
	grades = new int[n];
	for(int i=0;i<n;i++)
		grades[i] = a.grades[i];
}

[LGOOZ1]

#include "stdafx.h"
#include <string>
#include <iostream>
#include <sstream>
using namespace std;

class student
{
	char *name;
	int *grades;
	int n,age; // amount of grades and age
public:
	student(char *nm="", int x=0, int a=0);
	student(const student & a);
	~student();
	void print() const;
	student operator+(const student &vec);
	student & operator=(const student & vec);
	friend int min(const student &vec);
};

student::student( char *nm/*=""*/, int x/*=0*/, int a/*=0*/ )
{
	name = new char[sizeof(nm)+1];
	strcpy(name,nm);
	n = x ; // # of grades
	grades = new int[n];
	age = a ;

	string tempString;
	int    tempNumber;

	for (int i=0;i<n;i++)
	{
		while(true)
		{
			cout << "Enter grade number " << i+1 << ": ";
			getline(cin,tempString);
			stringstream tempStream(tempString);
			if(tempStream>>tempNumber)
			{
				grades[i] = tempNumber;
				break;
			}
			cout << "You've entered an invalid grade, please try again" << endl;
		}
	}
}

student::student( const student & a )
{
	n = a.n;
	age = a.age;
	name = new char[sizeof(a.name)+1];
	strcpy(name,a.name);
	grades = new int[n];
	for(int i=0;i<n;i++)
		grades[i] = a.grades[i];
}

student::~student()
{
	delete[] name;
	delete[] grades;
	name = NULL;
	grades = NULL;
}

void student::print() const
{
	cout << "Name: " << name << "\n";
	cout << "Age: " << age << "\n";
	for (int i=0;i<n;i++)
		cout << grades[i] << " ";
	cout << endl;
}

student student::operator+( const student &vec )
{
	student temp( *this ); // copy constructor

	if (age > vec.age)
		temp.age = age;
	else
		temp.age = vec.age;

	if (strlen(name) > strlen(vec.name))
	{
		temp.name = new char[strlen(name)+1];
		strcpy(temp.name,name);
	}
	else
	{
		temp.name = new char[strlen(vec.name)+1];
		strcpy(temp.name,vec.name);
	}

	if (n > vec.n)
	{
		temp.n = n;
		temp.grades = new int[temp.n];
		for(int i=0;i<n;i++)
			temp.grades[i] = grades[i];
		for(int i=0;i<vec.n;i++)
			temp.grades[i]+= vec.grades[i];
	}
	else
	{
		temp.n = vec.n;
		temp.grades = new int[temp.n];
		for(int i=0;i<vec.n;i++)
			temp.grades[i] = vec.grades[i];
		for(int i=0;i<n;i++)
			temp.grades[i]+= grades[i];
	}

	return temp;
}

student & student::operator=( const student & vec )
{
	if ( &vec != this )
	{
		delete[] name,grades ;
		n = vec.n;
		age = vec.age;
		name = new char[strlen(vec.name)+1];
		strcpy(name,vec.name);
		grades = new int[vec.n];
		copy(vec.grades,vec.grades+n,grades); // also works, 1 line
		//for (int i =0 ; i < n ; i++)
		//	grades[i] = vec.grades[i];
	}

	return *this;
}

int min(const student &vec)
{
	if (vec.n==0)
		return -1;

	int tempGrade = vec.grades[0];

	for (int i=0;i<vec.n-1;i++)
	{
		if (vec.grades[i] < tempGrade)
			tempGrade = vec.grades[i];
	}

	return tempGrade;
}

[ne555]

http://www.cplusplus.com/forum/general/112111/

> student::operator+
that's an abuse.
I don't see how can you add two students. Your code seems to be taking portions of each parameter.


> name = new char[sizeof(a.name)+1];

sizeof(a.name) is known at compile time, by instance it may be 4.
Notice how in other parts you use 'strlen()'

[LGOOZ1]

This is an exercise from a book, so naturally at this stage some less than ideal concepts are being exercised to prove a point or something similar :)

Adding the two students means to take the longer name, higher age and longer grade list + adding the shorter grade list to the new one. This is actually performed as should and I can see the output is as desired if I ignore the error.

yes - the sizeof was actually a mistake!!! :) Thanks