Copy constructors for vectors of pointers

[Golden Lizard]

I'm trying to make a copy constructor for the following class, but since I got a pointer vector of pointers (I know it sounds silly =p) I don't know if it's enough to just copy the vector itself or do I have to loop through all variables contained in the vector and copy them?

//from hand.h
 class hand
{
    public:
	        hand();
	        virtual ~hand();
	        hand(const hand & aHand);

		void               virtual create   () const =0;
		std::vector<card*> virtual acc_cards();
		void               virtual add_card (card * crd);
        
    protected:
		std::vector <card*> * cards;

    private:
		void discard(int const &nr);
		void draw   (int const &nr);
		int check   ();

};

In the destructor I loop through all elements to delete them from the heap, then delete the vector itself.

//from hand.cpp
hand::~hand()
{
	std::vector<card*>::const_iterator *it;
	for (it = &cards->begin(); it != &cards->end(); ++it)
	{
		delete it;
		it = NULL;

	}

	cards->clear();
	delete cards;
	cards = NULL;

};

but I use this as my copy constructor. Is this enough or I should copy all elements from the vector and assign them to the heap as well?

hand::hand(const hand& aHand)
{
	cards = new std::vector<card*>(*aHand.cards);

};

Thanks in advance

[keskiverto]

Let's see:

#include <iostream>
#include <vector>

int main() {
  int foo = 42;

  std::vector<int*> bar;
  bar.push_back( &foo );

  std::vector<int*> gaz( bar );

  std::cout << &foo << '\n';
  std::cout << bar.front() << '\n';
  std::cout << gaz.front() << '\n';
  return 0;
}

Do you feel lucky?

Each vector contains one pointer. Those pointers hold same address. The address of foo. Of course they do; the pointer in gaz is a copy of the pointer in bar.

However, that copy construction did not create a new object as copy of foo, nor update the pointer in gaz to point to such object. In your program a destructor of each would attempt to delete the one and only pointed to object.

In other words, if you do want to make deep copies, then you have to make those copies.


PS. It is late hour, but your destructor looks extremely fishy. In fact, I do bet that it will crash in awesome ways.

[Golden Lizard]

So I have to copy every element in order to assure that the copy constructor doesn't make any shallow copies.


Also, what's wrong with the destructor? Don't I get rid of all dangling pointers this way? Changed it from a pointer to a vector to a simple vector of pointers, it's better this way?

hand::~hand()
{
	std::vector<card*>::const_iterator *it;
	for (it = &cards.begin(); it != &cards.end(); ++it)
	{
		delete it;
		it = NULL;

	}

	cards.clear();

};

[coder777]

You don't need a pointer to the vector. If you want to copy hand you need a copy constructor and operator (operator=const hand & aHand);). If you don't want the copy make it private.

I bet you don't need a pointer to card either. If you don't have pointers copy is no problem at all.

The delete in the destructor is wrong:

hand::~hand()
{
	std::vector<card*>::const_iterator it; // Not a pointer
	for (it = cards.begin(); it != cards.end(); ++it)
	{
		delete (*it); // Note: (*it) you don't want to delete the iterator but its content
		it = NULL; // Unnecessary

	}

	cards.clear();

}

[Golden Lizard]

Thanks

[Gamer2015]

You should try to avoid pointers as often as possible(especially raw pointers).
I don't know what you're working on but since you asked it's better for you to just have a vector of objects, not pointer to objects.