-1

I have an extraction operator being used for a class that has a char* member 'name'. Here is my code from my main driver:

  Player tempPlayer;
  for(int i=0;i<4;i++){
     fin >> tempPlayer;
     }

I then go on to do something with that extracted player (which is irrelevant), but the issue is that each time the extraction operator is used, something strange occurs. Here is the definition of the operator:

ifstream& operator>>(ifstream& fin, Player& currentPlayer){
   char* temp = new char[50];
   char tempChar;
   fin >> temp;
   // importing from a file that contains names of about 6 characters each
   stringCopy(currentPlayer.name, temp);
   delete[] temp;
   temp = NULL;
   return fin;
   }

stringCopy body:

 void stringCopy(char *destPtr, const char *sourcePtr){
     while(*sourcePtr!='\0'){
        *destPtr = *sourcePtr;
        destPtr++;
        sourcePtr++;
        }
     *destPtr='\0';
     }

I have been debugging by printing out the memory addresses being used for temp and the name.

The FIRST time the extraction operator is called, the player's name and the temp array have different memory addresses, which is what should happen. Then temp is then deleted and set to NULL, which I confirmed by printing the address (and getting '0'), and the address of the player's name remains after the function has returned.

However, on subsequent calls, the address of both temp AND the player's name become identical to the address of the first player's name. The name address SHOULD be the same, as it is the same object that is just being overwritten, but why is temp getting the SAME address as "name" if it is allocated with the new char[] keywords?

Here is some code I used when debugging:

Along each step of the way in the body of the operator:

cout << "temp address followed by name address: " << (void*)temp << " " << (void*)(currentPlayer.name) << endl;

In the main driver:

cout << "player " << i+1 << " has been extracted with name address " << (void*)(tempPlayer.name) << endl;

Here is the Player constructor:

Player::Player(){
   name = new char[50];
   stringCopy(name,"name");
   ID = new int[5];
   }

Excluding irrelevant data members, here is Player definition:

class Player{
   public:
      char* name;
};
10
  • Show us the code for your stringCopy function. You should probably be using std::string if you're using C++ anyway. Dec 4, 2013 at 2:12
  • Why do you say that stringCopy does a 'deep' copy? Is name an object too? Please post stringCopy and Player.
    – KeithSmith
    Dec 4, 2013 at 2:18
  • So, is there actually a problem? If dynamic memory is allocated to a now free but once upon a time allocated memory location, there's no need for concern. Dec 4, 2013 at 2:18
  • the declartion for 'name' is simply char* name
    – Bobazonski
    Dec 4, 2013 at 2:18
  • 1
    Show us the constructor for Player. I wouldn't be surprised to see name isn't initialized and is picking up garbage off the stack, for example.
    – Joe Z
    Dec 4, 2013 at 2:27

1 Answer 1

0

When you delete[] something you've new[]ed, the standard library is allowed to reuse that memory. It's not surprising, therefore, that temp ends up getting the same address again and again. That is expected behavior and not a problem in and of itself.

The real problem is that in your "driver" code (a bit of code you haven't shown us), you have a shallow copy of tempPlayer somewhere. This copies the pointer to name, without allocating new storage. When that copy gets deleted, the destructor for Player deletes name in this shallow copy, releasing name to the heap.

Now the original is left pointing to freed memory. A future call to your operator>> then allocates this now supposedly-free memory. Oops!

Your short term fix was to remove the shallow copy. The long term, correct fix is to implement proper copy and copy-assignment constructors, as per the "Rule of 3" ( http://en.wikipedia.org/wiki/Rule_of_three_%28C++_programming%29 ), or if you want to be really C++11 friendly, the rule-of-5. (Same link.)

Alternately, you could consider making the copy and copy-assignment constructors private, which would prevent unwanted copies of these objects. (Some hints here: What's the most reliable way to prohibit a copy constructor in C++? )

11
  • I didn't originally state it very clearly in the question, but the issue is that on the second call of the line that allocates memory to temp, the address of the new memory is the SAME as the already-in-use name array.
    – Bobazonski
    Dec 4, 2013 at 2:24
  • Right, and there's no problem that temp gets the same memory, because you've freed it for reuse with delete[]. Or are you saying that &currentPlayer.name == temp? That shouldn't happen. Show us the output printed by your debug code.
    – Joe Z
    Dec 4, 2013 at 2:25
  • @user1362548: so show us the player class and how the memory for .name is allocated / deallocated.... Dec 4, 2013 at 2:26
  • @Joe Z, this is what I mean. The output for first call: temp address followed by name address: 0x8ed500 0x8ea2e0 after setting temp to NULL: temp address and name address: 0 0x8ea2e0 and then on second call: temp address and name address: 0x8ea2e0 0x8ea2e0 then after second set to null: temp address and name address: 0 0x8ea2e0
    – Bobazonski
    Dec 4, 2013 at 2:31
  • By the way, where the comments are located in your answer code, I am doing something here, I just knew it was irrelevant to the question being asked.
    – Bobazonski
    Dec 4, 2013 at 2:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.