-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;
};
Bobazonski
  • 1,515
  • 5
  • 26
  • 43
  • Show us the code for your stringCopy function. You should probably be using std::string if you're using C++ anyway. – Jesus Ramos Dec 04 '13 at 02:12
  • Why do you say that `stringCopy` does a 'deep' copy? Is `name` an object too? Please post `stringCopy` and `Player`. – KeithSmith Dec 04 '13 at 02: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. – Fiddling Bits Dec 04 '13 at 02:18
  • the declartion for 'name' is simply `char* name` – Bobazonski Dec 04 '13 at 02:18
  • It is NOT freed memory. It is still in use for the name, and being allocated to "temp" at the same time for some reason -- I didn't state this very clearly in the question so I've revised where I said that. – Bobazonski Dec 04 '13 at 02:19
  • Where is the storage for `Player.name`? It is a pointer. It must point to something. Does the constructor of `Player` allocate storage for `name'? – KeithSmith Dec 04 '13 at 02:22
  • Why are you "extracting" into the _exact same object_ 4 times, but expecting it somehow to change addresses? – Joe Z Dec 04 '13 at 02:23
  • I am not expecting the address of `name` to change. I am expecting the second allocation of `temp` NOT to match the address of `name` which is already in use at that point. – Bobazonski Dec 04 '13 at 02:25
  • 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 04 '13 at 02:27
  • Given `tempPlayer` is created directly before the streaming operation and the constructor allocates properly, the only way `.name`'s address can also be returned by `temp = new char[50]` is if you've corrupted your heap earlier. My suggestion: post a minimal complete compilable program demonstrating the problem, or stop using `char*`s and use `std::string` - it's 1000 times harder to stuff up. – Tony Delroy Dec 04 '13 at 02:38

1 Answers1

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++? )

Community
  • 1
  • 1
Joe Z
  • 17,413
  • 3
  • 28
  • 39
  • 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 04 '13 at 02: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 04 '13 at 02:25
  • @user1362548: so show us the player class and how the memory for .name is allocated / deallocated.... – Tony Delroy Dec 04 '13 at 02: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 04 '13 at 02: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 04 '13 at 02:37
  • It looks like your `name` address is fine, and the same across all four calls (what I'd expect passing the same object four times). Your `temp` address was NULL twice, which seems very weird. Where exactly are you printing `temp` from? – Joe Z Dec 04 '13 at 02:38
  • Yopu're right about the `name` address, and I've been aware of this. The NULL is intentional behavior. I used the debugging line given in my question at two places in the operator body: once right after the stringCopy and once right before the return. – Bobazonski Dec 04 '13 at 02:46
  • Ok, so now I suspect your destructor for `Player` is `delete[]`ing `name`, but you have an unintentional copy of `Player` that is using the compiler's default (shallow) copy constructor. When that copy destructs, it `delete[]` name, but because it's a shallow copy, it deletes the area that's also pointed to by the original. All that is probably in your "uninteresting" code that you've omitted. – Joe Z Dec 04 '13 at 02:47
  • I have made a stupid mistake... there is one line of "uninteresting" code within the for loop that I omitted, but thought I had commented out, which was causing the issue. I was enqueuing the player to a node-based list but the node initializer data parameter was not `const` or passed by reference, causing the issue all along. – Bobazonski Dec 04 '13 at 02:56
  • I don't think `const` would have saved you. It would have at best papered over the issue if the compiler happened to optimize the copy out. Probably ought to add a copy constructor, eh? (See "Rule of 3." http://en.wikipedia.org/wiki/Rule_of_three_%28C++_programming%29 ) – Joe Z Dec 04 '13 at 03:04
  • I'll edit my answer above to reflect the real problem—that you had a shallow copy that was calling `delete[]` and the correct fix is to implement the rule-of-3 to prevent this happening again, or make the copy and copy-assignment constructors private. – Joe Z Dec 04 '13 at 03:09