Defining the copy constructor and copy assignment operator for a user-defined vector

[Frank14]

Hi guys,

Please take a look at this code. It's under construction still.

#include <std_lib_facilities_4.h>
using namespace std;

template <class T> class pvector 
{
public:
	pvector() = default;
	pvector(const pvector& N)  // Copy constructor
	{
		for (const auto& v : N.vec)
			push_back(v);
	}
	
	pvector& operator= (const pvector& N)  // Copy assignment operator
	{
		auto l = N;
		std::swap(l.vec, vec);
		return *this;
	}

	~pvector() { 
		for (auto ptr : vec) 
			delete ptr;
	}

	auto size() const { return vec.size(); }
	void push_back(T* ptr) { vec.push_back(ptr); }
	void pop_back() { delete vec.back(); vec.pop_back(); } 
	 
	T*& operator[] (std::size_t pos) { return vec[pos]; }
	const T*& operator[] (std::size_t pos) const { return vec[pos]; }

private: 
	vector<T*> vec; // vector of pointers
};

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

int main() {
	
	pvector<int> v1;
	for (size_t i = 0; i < 10; i++)
		v1.push_back(new int (2*i));

	pvector<int> v2 = v1;
	for (size_t i = 0; i < v2.size(); i++)
		cout << *v2[i] << ' ';
	cout << endl;
	
	system("pause");
	return 0;
}

v2 is a copy of v1. When the program terminates, the destructor will be revoked, and it deletes the elements of both pvectors. But why do I get an exception error at the delete ptr; line of the destructor before ending the program totally, please?

And couldn't I use this:

vec = N.vec;
return *this;

instead of the code inside the copy assignment operator? This revokes the copy assignment operator of std::vector and does the job. Isn't it?

[keskiverto]

v2 is a copy of v1
v2.vec is a copy of v1.vec
v2.vec[0] is a copy of v1.vec[0]

BUT

*(v2.vec[0]) is the same object as *(v1.vec[0])

[Frank14]

Yes, I know, because the two point to the same object. But I asked about the error I face and also re-implementing the copy assignment operator!
But thanks for the reply.

[keskiverto]

I asked about the error I face

At the end of main() v2 and v1 are destroyed. Each calls destructor.
Each call of destructor attempts to delete the objects that both pvectors point to.
Double delete is an error.

Currently you make only a shallow copy; you copy pointers but not objects. However pvector thinks that it owns the objects (i.e. attempts to delete).

If you would use std::shared_ptr and not raw painters, then the shared_ptr would do the delete for you and avoid the double delete.


You could choose to do a deep copy; create copies of pointed to objects.


It is the copy constructor that you need to focus on, for you implement copy assignment with that constructor.


[edit] Duthomas has nice illustration: http://www.cplusplus.com/forum/beginner/221871/#msg1018417

[JLBorges]

There is no sane way by which you can make this pvector<T> CopyConstructible or CopyAssignable. (Why?)

It is quite easy, and logical, to make it MoveConstructible and MoveAssignable.

[Frank14]

Currently you make only a shallow copy

I modified it to this:

#include <std_lib_facilities_4.h>
using namespace std;

template <class T> class pvector 
{
public:
	pvector() = default;
	pvector(const pvector& N)  // Copy constructor
	{
		for (const auto& v : N.vec)
			push_back(new T(*v));
	}
	
	pvector& operator= (const pvector& N)  // Copy assignment
	{
		auto l = N;
		swap(l.vec, vec);
		return *this;
	}

	~pvector() { 
		for (auto ptr : vec) 
			delete ptr;
	}

	auto begin() const { return vec.begin(); }
	auto end() const { return vec.end();  }
	auto size() const { return vec.size(); }
	void push_back(T* ptr) { vec.push_back(ptr); }
	void pop_back() { delete vec.back(); vec.pop_back(); } 
	 
	T*& operator[] (std::size_t pos) { return vec[pos]; }
	const T*& operator[] (std::size_t pos) const { return vec[pos]; }

private: 
	vector<T*> vec; // vector of pointers
};

Now I can use the range-based for for pvector too.

Duthomas has nice illustration: http://www.cplusplus.com/forum/beginner/221871/#msg1018417

Very good explanation. I only didn't understand this part well:
Implementing a deep copy is required when you know that you are the sole keeper of some referenced data

[Frank14]

There is no sane way by which you can make this pvector<T> CopyConstructible or CopyAssignable. (Why?)

Yes, why?

It is quite easy, and logical, to make it MoveConstructible and MoveAssignable.

I haven't been taught this constructors/operators. So it wouldn't be that easy to use it here in that level I think.

[JLBorges]

> Yes, why?

Consider the copy constructor: since the destructor of the copy too would delete the pointers, how do we ensure that two different copies of a pvector would not try to destroy the same set of objects?

This appears to be an easy way out: "You could choose to do a deep copy; create copies of pointed to objects."

Till you realise that there is no sensible way for pvector as written to make copies of the pointed to objects.
What does it do if the type T is not CopyConstructible?
What does it do if the static type T is a base class at the top of an inheritance heirarchy?

[Frank14]

Consider the copy constructor: since the destructor of the copy too would delete the pointers, how do we ensure that two different copies of a pvector would not try to destroy the same set of objects?

I don't know too much details but I think I've used a deep copy and it's fine for deleting as I've tested it.
Is there any problem with the code now?

What does it do if the type T is not CopyConstructible?

I've used my own constructor not default, but what types do you mean please?

[JLBorges]

> it's fine for deleting as I've tested it.
> what types do you mean please?

Try compiling this:

#include <mutex>

struct A // thread safe objects (not CopyConstructible)
{
    std::mutex m ; // http://en.cppreference.com/w/cpp/thread/mutex

    // ...
};

int main()
{
    my::pvector<A> vec ;
    
    // add file items
    for( int i = 0 ; i < 5 ; ++i ) vec.push_back( new A )  ;
    
    // make a copy of the pvector (copy constructor)
    my::pvector<A> copy_of_vec(vec) ;

}

[keskiverto]

Your code does now: push_back(new T(*v));

Let me now write:

class NonAssignable {
public:
             NonAssignable( NonAssignable const& ) = delete;
  NonAssignable& operator=( NonAssignable const& ) = delete;
};

int main() {
  pvector<NonAssignable> fubar;
  // Can we make a copy of fubar?
}

If you do attempt a copy, you will call new NonAssignable( *v ) where *v is a NonAssignable. That is not possible, for my NonAssignable has no copy constructor.

There is occasionally a need for classes that are like the NonAssignable. There can be a need to store many objects too.


Lets make the other example too:

struct Foo {
 int x;
};

struct Bar : public Foo {
  int y;
};

int main() {
  pvector<Foo> snafu;
  snafu.push_back( new Bar );

  pvector<Foo> fubar( snafu );
  // what does fubar have?
}

A: a copy of sorts, just like in the following:

int main() {
  Foo* snafu = new Bar;
  Foo* fubar = new Foo( *snafu );
  // what does fubar have? A Foo. Foo is not Bar.
  // fubar has the copy of snafu.x.
  // dynamic_cast<Bar*>(snafu)->y was not copied.
}

[Frank14]

Try compiling this:

The code has errors. And also the stuff is too advanced for me. I'm focusing on lower level matters. I appreciate you for your help.

[Frank14]

snafu.push_back( new Bar );
Here I think, we can't add objects of type Bar to the pvector of type Foo because Bar includes/has Foo but Foo doesn't include/have Bar.

pvector<Foo> fubar( snafu );
  // what does fubar have? 

It's very complex for me. I've many weaknesses in using inheritance well.

Foo* snafu = new Bar;
I too think this isn't right because of the reason above.

[keskiverto]

Public inheritance means a IS-A relationship. Not a HAS-A. Bar inherits from Foo and therefore every Bar object is a Foo object. It is perfectly legal to store the address of Bar object in a Foo*.

You don't store objects in pvector. You store pointers to objects.

[Frank14]

I should read about C++ inheritance well.