Recently I got a task to do in C++, implement a Set class with union, intersection etc. as overloaded operators. I've got a problem with overloading an operator+(). I decide to use vectors and get the advantage of some algorithm's library functions. The problem is I HAD TO pass to constructor an array pointer and array size. This complicated this task a bit... I can compile it but during the "z=a+b" operation I encounter somekind of memory leak. Can anyone explain me what am I doing wrong?
class Set {
int number; // array size (can't be changed)
int *elems; // array pointer (same)
public:
Set();
Set(int, int*); // (can't be changed)
~Set();
friend Set operator+(const Set& X,const Set& Y){
std::vector<int> v(X.number+Y.number);
std::vector<int>::iterator it;
it=std::set_union (X.elems, X.elems+X.number, Y.elems, Y.elems+Y.number, v.begin());
v.resize(it-v.begin());
Set Z;
Z.number=v.size();
Z.elems=&v[0];
return Z;
}
};
Set::Set(){};
Set::Set(int n, int* array){
number=n;
elems = array = new int[number];
for(int i=0; i<number; i++) // creating Set
std::cin >> elems[i];
std::sort(elems, elems + number);
}
Set::~Set(){
delete[] elems;
}
int main(){
int* pointer;
Set z;
Set a = Set(5, pointer);
Set b = Set(2, pointer);
z=a+b;
}
I added copy constructor and copy assingment, changed the operator+() as NathanOliver advised and now I am passing to constructor static array. Still have memory leak and strange thing is that I got this memory leak even when in main there is only class variable initialization, doesn't matter if with parameters or not... Any suggestions? I think cunstructor is valid.
Set::Set(int n, int* array){
number = n;
elems = array;
std::sort(elems, elems + number);
}
Set::Set(const Set& s){
number=s.number;
elems=s.elems;
}
Set& operator=(const Set& X){
if(this==&X)
return *this;
delete [] elems;
elems=X.elems;
number=X.number;
return *this;
I use gcc (tdm64-2) 4.8.1 compiler.
In
friend Set operator+(const Set& X,const Set& Y){
std::vector<int> v(X.number+Y.number);
std::vector<int>::iterator it;
it=std::set_union (X.elems, X.elems+X.number, Y.elems, Y.elems+Y.number, v.begin());
v.resize(it-v.begin());
Set Z;
Z.number=v.size();
Z.elems=&v[0];
return Z;
}
You create a vector, modify it and then set the elems to point to what the vector contains. The issue with that is that when the vector is destroyed at the end of the function the memory that the vector held is released. So you now have a pointer pointing to memory you no longer own. Trying to do anything with it is undefined behavior. What you could do is create a new array, copy the elements of the vector into the array and then assign the new array to `elems
Set Z;
Z.number= v.size();
Z.elems= new int[z.number];
for (int i = 0; i < Z.number; i++)
Z.elems[i] = v[i];
return Z;
Secondly you need to define a copy constructor and assignment operator for you class. To do that reference: What is The Rule of Three?
The best solution (but I can't tell if you're allowed to do this specifically) is to use vector internally in your Set and assign it from the passed in pointer and length using the two-iterator constructor.
Now if that's not possible you need to properly manage the memory of your class:
- You need a copy constructor and copy assignment operator.
- In your
operator+ you can't create a local vector and then take the address of its memory, that memory will disappear as soon as the operator returns.
- Possibly other things I didn't catch.
When you have z=a+b, the assignment operator for the Set class is used. You didn't define a custom version of this operator, so the default compiler-generated one is used. This compiler-generated assignment operator=() simply does a member-wise copy.
Since you have raw owning pointers in your Set class, this doesn't work correctly: the default compiler-generated operator=() shallow-copies pointers, instead you should deep-copy the data.
An option to fix this is to define your own version of operator=(), paying attention to do proper deep-copy of source data.
Note that in this case you also should define a copy constructor.
But a better option would be to get rid of the owning raw pointer data members, and instead use a RAII building block class, like std::vector.
So, for example, instead of these data members:
int number; // array size (can't be changed)
int *elems; // array pointer (same)
you could just have a single:
std::vector<int> elems;
If you do that, the default compiler-generated operator=() will work just fine, since it will copy the std::vector data member (not the raw owning pointers), and std::vector knows how to properly copy its content without leaking resources.
