Dynamic memory, copy constructors, deconstructors and return values

[wasabi]

I have, over the last few weeks developed a code for my internship that allows for efficient data-gathering from a mastodon of a text file (I mean, a 150Mb .txt is pretty substantial, right?). Anywho. That part is working fine. Or, well, it was until I started fidgeting with some segments of the code in order to unify some things into some structs I have. Sure, classes are private or whatnot, but I come from a C-heavy background and like the way the word struct sounds. Struct.

I have also searched cplusplus.com and other forums for answers to this question (which I'm sure has been answered before, but I just can't for the life of me find it).

So, here's the code.

struct elem
{
	unsigned int* elements;
	unsigned int num;
	elem();
	elem(const elem &obj);
	~elem();
};

elem::elem(const elem &obj)
{
	num=obj.num;
	elements=new unsigned int[obj.num];
	*elements=*obj.elements;
}
elem::elem()
{
	num=0;
	elements=NULL;
}
elem::~elem()
{
	delete[] elements;
}

elem elements()
{
        elem elem;
        //opens a file from which elem captures values of num and elements. elements is also allocated with new here.
        return elem;
}

int main()
{
	elem elem;
        elem=elements();
        return 0;
}

Basically, this struct is returned by a function with certain values in num and elements. The values of that struct are therefore transferred to the temporary result-carrying object and the original struct is sent to the d'tor. The temporary object is then returned to main. It then seems to transfer the value of num before going to the d'tor as well. However, the elements array does not seem to the transferred. This, in turn means the value of the array is also d'tor'ed. Only then is a value returned to elem in main. Which means that when main returns, the d'tor is summoned once more, giving an error.

Using the step processes (I'm on VC++, so the shortcut is F10,F11), I can basically see it go from
return elem
copy constructor
back to 'return elem'
d'tor
elem=elements() in Main
d'tor
back to Main
and error at 'return 0'

Should I check the value of num just prior to the error, I can see it is correct. Elements, however, is clearly pointing to nothing. Which is precisely the problem.

I am sure that what I'm doing is a silly, silly mistake, but I can't for the life of me find it. It is worth mentioning that those copy constructors and d'tors are simply some of many I've tried so far. They're just the only ones that even get as far as return 0. The others would fail already after reaching the d'tor immediately after landing onto Main the first time.

[guestgulkan]

We can see what is pretty much going on here - but can you correct your code first - you seem to have some confusion with the names elements and elementos

[wasabi]

Done. My bad. I just so happen to be Brasilian (with an S, I tell you!), so the 'names' are all given in Portuguese. So much for trying to make life easier for others... Woops.

[guestgulkan]

The problem here is in the copy constructor:

elem::elem(const elem &obj)
{
	num=obj.num;
	elements=new unsigned int[obj.num];
	*elements=*obj.elements;
}

If the object being copied is an elem object created using the default constructor - then

	
num=0;
elements=NULL;

So to say elements=new unsigned int[obj.num]; in your copy constructor
will create 0 (ZERO) amount of integers and elements will be NULL. Then to say
*elements=*obj.elements; is a double fault because you will be trying to dereference two NULL pointers. CRASH


PS
Even if you did have two good pointers and some integers to transfer - *elements=*obj.elements; will only transfer 1 (one) of them - you cannot transfer whole arrays like this.
You will need to do a loop and transfer each integer.

[wasabi]

All the objects are initialized at num=0 and elements=NULL, but it is impossible for them to reach the end of the function (and therefore the copy constructor) without having num!=0 and elements!=NULL.

I might have forgotten to mention this in the OP, but every time but the last that the copy constructor is used, I can see the values of num and elements are correct. It's only on the last iteration (elem=[results from]elements()) that the value of elements disappears.

And thanks about the hint regarding the loop... I'd forgotten that... but the main problem still remains.

At the end of the function, the values are num=2, elements = {1773,2049}. However, right before Main's return 0, the values are num=2, elements= {two very large numbers. Clearly trash}.

[guestgulkan]

I suppose if in the elements() function, you did actually read some data into elem then the error I mention would not have occurred in this case .

But I think you still need to modify the copy constructor to guard against copy constructing from a default constructed elem.

[wasabi]

While that is true (especially given how the program isn't done, so I might make a booboo later on), it is not my primary concern right now, given how even the case where it is initialized doesn't work. I don't have the code in front of me right now, but I have built the loop for all the integers in elements.

I've been thinking about this all day and can't see what's wrong with the code (for cases where both num and elements already have been initialized)... any more pointers? Keep in mind that I'm actually painfully new to C++ (started reading up less than two weeks ago, but have beginner-intermediate C knowledge), so I might have made some pretty basic, basic mistakes.

EDIT: Anyone? Pretty please?

[guestgulkan]

Can you just post all the code you have now - that would speed matters up considerably.

[wasabi]

Ask and you shall receive, but one should be wary what one wishes.

Or, not. Given how the code's too long for one post. So I'll cut it down to only the interesting stuff

The following is the header file:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <math.h>
#include <iostream>
#include <string>
using namespace std;

struct result
{
	int elem_ratio;
	int step_ratio;
	int elem_sigy_1;
	int step_sigy_1;
	int step_sigy_t;
	result(void);
};
struct elem
{
	unsigned int* elementos;
	unsigned int num;
	elem();
	elem(const elem &obj);
	~elem();
};
FILE* acha_titulo(FILE* f, char* limite); //pesquisa o arquivo ate achar o limite
FILE* passos(FILE* f); //move o *f ate os resultados de interesse no pos
FILE* acha_elem(FILE* f, int elem2); //move o *f ate um elemento de interesse
FILE* capta_res(FILE* f, int elem, int gp, int step, result* results, char* param); 
int capta_res(FILE** fp, int gp, result* results); //diz se sigy=0. Usado para checar sigy_t
FILE* prox_step(FILE* f,int n_elem, int node); //move *f ao proximo step
result resultados(FILE* f, unsigned int* l_elem, unsigned int num); //acha todos os resultados
FILE* abre_f(void); //abre o arquivo de interesse
int compara(const void* a, const void* b); //usado para qsort dos elementos de interesse
elem elementos(void); //coleta e ordena os elementos de interesse do teclado
elem elementos(int); //coleta os elementos de interesse de um arquivo 

And now the code file.

#include "busca.h"
using namespace std;
result::result(void)
{
	step_ratio=0;
	elem_ratio=0;
	step_sigy_1=0;
	elem_sigy_1=0;
	step_sigy_t=0;
}
elem::elem(const elem &obj)
{
	unsigned int i;
	num=obj.num;
	elementos=new unsigned int[obj.num];
	for(i=0;i<num;i++)
		elementos[i]=obj.elementos[i];
}
elem::elem()
{
	num=0;
	elementos=NULL;
}
elem::~elem()
{
	delete[] elementos;
}
FILE* acha_titulo(FILE* f, char* limite);
FILE* passos(FILE* f);
FILE* acha_elem(FILE* f, int elem2);
FILE* capta_res(FILE* f, int elem, int gp, int step, result* results, char* param);
int capta_res(FILE** fp, int gp, result* results);
FILE* prox_step(FILE* f,int n_elem, int node);
result resultados(FILE* f, unsigned int* l_elem, unsigned int num);
FILE* abre_f(void);
int compara(const void* a, const void* b);
elem elementos()
{
	elem elem;
	char buff[1001];
	char*  cnum;
	int i,j=1,k=0,m;
	cout << "Digite os elementos de interesse, separados por virgula (1,2,3,4,5...)" << endl;
	cin >> buff;
	cout << endl;
	for(i=0;i<1001 && buff[i]!='\0';i++)
		if(buff[i]==',')
			j++;
	elem.num=j;
	j=0;
	elem.elementos=new unsigned int[elem.num];
	for(i=0;buff[i]!='\0' && i<501;i++)
	{
		if(i!=0)
			i++;
		while(buff[j+i]!=',' && buff[j+i]!='\0')
			j++;
		cnum = new char[j+1];
		for(m=0;m<j;m++)
			cnum[m]=buff[m+i];
		cnum[j]='\0';
		elem.elementos[k]=atoi(cnum);
		k++;
		i+=j-1;
		j=0;
		delete[] cnum;
	}
	qsort(elem.elementos,elem.num,sizeof(int),compara);
	cin.ignore();
	return elem;
}
elem elementos(int);

And now Main.cpp:

#include "busca.h"

int main()
{
	FILE* f;
	result results;
	char i;
	elem elem;
	/*f=abre_f();
	cout << "Deseja usar um arquivo para o input de elementos de interesse? <Y/N>" << endl;
	cin >> i;
	if(i=='Y' || i=='y'){}
		elem=elementos(0);
	else*/
		elem=elementos();
	//unsigned int elem[5]={1773,2049,2054,2079,2152}; //ordenado
	//f=passos(f);
	//results=resultados(f,elem.elementos,elem.num);
	//fclose(f);
	return 0;
}

I have only shown the code for elementos(void) because, as you can see from int main(), it is the very first function to be called and the program aborts without getting a chance to call anything else (I have tested using VC++'s Step-by-Step debug); I have commented abre_f() and elementos(int) because they do not have any effect whatsoever upon elementos(void). abre_f() opens the file from which I am going to get the results for the elements of interest (which obviously never happens, since the program aborts before even getting said elements) and elementos(int) gets the ID numbers of the elements of interest from another file. elementos(void), on the other hand, has the user type up the ID numbers by hand (separated by commas) and then saves the ID numbers in a dynamically allocated vector (elem.elementos*) along with the total number of elements (elem.num).

It goes without saying that any hints, tips and slaps across the face regarding my filthy code are more than welcome. Though I'd rather keep the slapping to a bare minimum.

[guestgulkan]

The problem is due to a lack of a suitable assignment operator.
With the code as written - The compiler will provide one - But as we have pointers this will
cause problems.

You need to write an assignment operator:
elem& operator=(const elem &obj)

[guestgulkan]

For the record - here is your program with the assignment operator added.
You will note there is no crash.
(P.S - I have got rid of the extra stuff - so we can clearly see what is going on.)

With assignment operator - NO crash.
Comment out the assignment operator stuff - CRASH

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <math.h>
#include <iostream>
#include <string>
using namespace std;

struct result
{
	int elem_ratio;
	int step_ratio;
	int elem_sigy_1;
	int step_sigy_1;
	int step_sigy_t;
	result(void);
};
struct elem
{
	unsigned int* elementos;
	unsigned int num;
	elem();
	elem(const elem &obj);
  elem& operator=(const elem &obj); //Assignment operator
	~elem();
};

//*************************************************************************************
result::result(void)
{
	step_ratio=0;
	elem_ratio=0;
	step_sigy_1=0;
	elem_sigy_1=0;
	step_sigy_t=0;
}

//********************************************************
elem::elem(const elem &obj)
{
	unsigned int i;
	num=obj.num;
	elementos=new unsigned int[obj.num];
	for(i=0;i<num;i++)
		elementos[i]=obj.elementos[i];
}

elem& elem::operator=(const elem &obj) //assignment operator definition
{
	unsigned int i;
  if (this != &obj) //important - check for self assignment 
  {
	  //if not self assignment
    delete [] elementos; //get rid of old stuff
    num=obj.num;
	  elementos=new unsigned int[obj.num];
	  for(i=0;i<num;i++)
		  elementos[i]=obj.elementos[i];
  }
  return *this;
}


elem::elem()
{
	num=0;
	elementos=NULL;
}
elem::~elem()
{
	delete[] elementos;
}

//***************************************************************


elem elementos()
{
	elem elem;
	char buff[1001];
	char*  cnum;
	int i,j=1,k=0,m;
	cout << "Digite os elementos de interesse, separados por virgula (1,2,3,4,5...)" << endl;
	cin >> buff;
	cout << endl;
	for(i=0;i<1001 && buff[i]!='\0';i++)
		if(buff[i]==',')
			j++;
	elem.num=j;
	j=0;
	elem.elementos=new unsigned int[elem.num];
	for(i=0;buff[i]!='\0' && i<501;i++)
	{
		if(i!=0)
			i++;
		while(buff[j+i]!=',' && buff[j+i]!='\0')
			j++;
		cnum = new char[j+1];
		for(m=0;m<j;m++)
			cnum[m]=buff[m+i];
		cnum[j]='\0';
		elem.elementos[k]=atoi(cnum);
		k++;
		i+=j-1;
		j=0;
		delete[] cnum;
	}
	//qsort(elem.elementos,elem.num,sizeof(int),compara);//Commented out for now
	cin.ignore();
	return elem;
}

//******************************************************************
int main()
{
	FILE* f;
	result results;
	char i;
	elem elem;

	elem=elementos();
	return 0;
}

[wasabi]

Thanks a bunch.

Now... I just don't really understand the need for the assignment operator (on a purely theoretical basis. As I've mentioned many a time before, I'm quite new to C++). I mean, I worked directly with the values of the struct, which worked just fine when I did this sort of thing in C. Or, I mean, maybe I was just memory hemorrhaging all over the place and didn't know it.

And my understanding of the check for self-assignment is because if you self-assign, you'll delete everything and then try to access it, correct?

And, actually, for that matter, what's an occasion of self-assignment? Why'd anyone use a=a;? Or am I just an idiot and don't know what self-assigment means?

[firedraco]

You want to protect yourself from that (yes it is a=a) just in case. You *shouldn't* have self assignment, but you want to make sure your class doesn't die if you have it happen.

[wasabi]

Ah. Okay. Good to know.

Gratzi mille, peoples. I still haven't run the program with the operator (don't have it on me), but if it ran for guestgulkan, then there's no reason why it shouldn't run on my program.

Don't worry, I'm sure I'll be back with questions in no time.

EDIT: Just giving a confirmation. Just plugged it in and it's working perfectly. Joy of joys.

Well, actually, I still don't understand why the assignment operator is necessary...

[jsmith]

In general, if you need a user-defined copy constructor, you need a user-defined assignment operator. They do essentially the same thing, just with different calling syntax:

elem e;
elem copy_e( e );   // copy construction

elem e2;
elem e2_copy = e2;  // also copy construction

elem e3, e4;
e4 = e3;  // assignment 

Your assignment operator would be MUCH simpler (not to mention exception safe) if you used the copy-swap idiom instead:

elem& elem::operator=( elem e )   // COPY rhs onto the stack using copy constructor
{
  // SWAP the data members of the copy (rhs) with the lhs.
  std::swap( num, e.num );
  std::swap( elements, e.elements );
  return *this;
}