Strange copy constructor behaviour, part 2

[hadoque]

Hello again.
I''m having another problem with the assignement I asked about yesterday.
I have two classes, ShipLoad and Player. Every Player object contains a ShipLoad. Every ShipLoad contains a string array, words, that contain a number of words. Everytime a ShipLoad object is copied the size of the words-array should be increased with 1.
The problem is now that when I add the second word to shipload, I get segmentation error, suggesting that the words-array is not expanded. But that does not seem to be it. If i start using a big words-array, that don't need extending, i still get the same error..
These are the relevant functions:

ShipLoad::ShipLoad()
{
  this->size = 1;
  words = new string[size];
  nrOfWords = 0;
}

ShipLoad::ShipLoad(const ShipLoad &obj)
{
  cout << "\nCopy constructor, size=" << this->size << endl;

  this->size = obj.size + 1;
  this->nrOfWords = obj.nrOfWords;
  words = new string[this->size];

for (int i = 0; i < obj.size; i++)
    this->words[i] = obj.words[i];
}


void ShipLoad::addWord(string word)
{
  cout << "\nAdding word at: " << this->nrOfWords << endl;
  cout << "\nSize is: " << this->size << endl;
  this->words[nrOfWords] = word;
  this->nrOfWords++;
}

bool Player::repeatLoad()
{
  bool success = true;
  string tempString;
  cout << "\nSpelare: " << getName();

  if (success)
    {
      cout << "\nAnge nytt ord: ";
      getline(cin,tempString);
      shipload->addWord(tempString);
    }

  return success;
  }

ShipLoad* Player::handOverShipLoad()
{
  ShipLoad temp = *shipload;
  ShipLoad *temp2 = &temp;
 return temp2;
}

void Player::reciveShipLoad(ShipLoad *aShip)
{
  shipload = aShip;
}

And this is how I do the testdrive:

#include "Player.h"
#include <string>
#include <iostream>

int main()
{
  ShipLoad *aShip = new ShipLoad();
  Player johan("Johan");
  Player trazan("Trazan");

  johan.reciveShipLoad(aShip);
  johan.repeatLoad();
  trazan.reciveShipLoad(johan.handOverShipLoad());
  trazan.repeatLoad();
  johan.reciveShipLoad(trazan.handOverShipLoad());
  johan.repeatLoad();

  return 0;
}

The complete source code is here: http://www.apspektakel.com/code/

[helios]

suggesting that the words-array is not expanded.

That's right. It isn't. Arrays can't be resized once they're created.
Why don't you just save yourself the trouble and use an std::vector<std::string>?

[hadoque]

Because the assignement says so... ;)
If I do:

string *words1  = new string[1];
words[0] = "word1";
string *words2  = new string[1];
words2[0] = words1[0];
delete [] words1;
string *words1  = new string[2];
words1[0] = words2[0];
delete [] words2;

I have expanded words1 to hold another word, right? This is what I want to do in the copy constructor. What am I doing wrong?

[helios]

Because the assignement says so...

Alright, then.

The way you're doing it, there, you're making one more copy than necessary.
A generic array resize looks like this:

T *temp=new T[new_size];
for (size_t a=0;a<old_size;a++)
    temp[a]=array[a];
delete[] array;
array=temp;

[hadoque]

Ok, but isn't this pretty much what I have done in the copy constructor? I don't get why you say that array words is not expanded..

ShipLoad::ShipLoad(const ShipLoad &obj)
{
  cout << "\nCopy constructor, size=" << this->size << endl;

  this->size = obj.size + 1;
  this->nrOfWords = obj.nrOfWords;
  words = new string[this->size];

for (int i = 0; i < obj.size; i++)
    this->words[i] = obj.words[i];
}

it doesn't matter if I copy the whole array at once instead of copying element by element...

[helios]

No. The copy constructor is only called when an instance is being constructed based on another instance. That's why it's called the copy constructor. For example:

  Player johan("Johan");
  Player trazan(johan); //Copy constructor being called. The line below is equivalent.
  Player trazan=johan;

When you call addWord(), the instance has already been constructed.

Also, the code in the copy constructor looks nothing like what I wrote above.

[hadoque]

Ok, what I wan't is for the array words to be expanded with 1 element when the object is copied, thats why I put the array expansion in the copy-constructor. So that after the copy is made, addWord() has room to add one word to the array.
I dont need the temp array, since I have two arrays, this->words and obj.words. I don't need to delete obj.words, since this is done in the destructor.

ShipLoad::ShipLoad(const ShipLoad &obj)
{
  this->size = (obj.size + 1); //increasing size (which is for the actual size of the array)
  this->nrOfWords = obj.nrOfWords; //copying nrOfWords (which is for the actual number of words entered)
  words = new string[this->size]; //creating new words array with increased size
  this->words = obj.words; //copying old array to new array
}

Is there still something wrong with my reasoning? cause i still get the segmentation error...

[helios]

Is there still something wrong with my reasoning?

Plenty.
Get this into your head: with the code you're using the copy constructor will not EVER be called.
You're not trying to create a new instance of ShipLoad. You're trying to enlarge an array that exists in an instance of ShipLoad that you've already created.
Consider this analogy: You have a fridge, and in it you have a glass of water (A). You need to put more water in your fridge, but you can't add more glasses. Do you buy a new fridge and put a bigger glass in it; or you get a bigger glass (B), pass the contents of A to B, and put B where A was?
This is exactly what you need to do in ShipLoad::addWord(). First, you create a bigger array (get B), copy the contents of the previous array into the new one (put A's water into B), delete the old array (but A in the cupboard), and assign this->words to the new array (put B in the fridge).

[hadoque]

But the copy constructor is used. I put a line in it printing to the screen when it is used, and it prints, just before addWord() is called.
I wrote handOverShipload(), so that it should use the copy constructor each time the object shipload is handed over to the next player...

ShipLoad* Player::handOverShipLoad()
{
  ShipLoad temp = *shipload;
  ShipLoad *temp2 = &temp;
 return temp2;
}

could the problem be in this function?

I also have an overloaded operator= for using with already created instances, but that one is not (yet?) used...

[Disch]

I haven't been following this thread... but yes, that handOverShipLoad function is all wrong.

You're returning a pointer to a temporary object. 'temp2' points to 'temp', but 'temp' is destroyed as soon as the function exists, which means you're returning a pointer to an object that no longer exists. IE: bad pointer.

[helios]

I didn't notice that before. handOverShipLoad() returns an invalid pointer. temp goes out of scope and is therefore destructed when the function returns.
Returning pointers or references to local objects is illegal.

Even if that wasn't the case, the size of the array is increased in the copy, not in the original (which itself makes no sense. It means the copy constructor doesn't make an exact copy of the parameter passed). You're trying to addWord() to the original, not to the copy.

By the way, what's with the inconsistent use of this->?

[hadoque]

Yeah, OK. That explains it. I better think this over once again.
My teacher uses this-> all the time in her constructors, I don't like it, but I used it just to be sure that wasn't the problem. I'm probably going to delete them, it doesn't look nice...

Thanks for all the help.