Quote: Original post by boolai
Do we still need to delete the TempPointer from memory?

It depends. In your case, NO.

Let's retrace your steps: you allocate memory dynamically within the function, then set an external pointer to reference that piece of memory. And then you delete the memory, leaving the external pointer (m_pPointer, which is the return value of the function) pointing at... what? What is the return value of your function?

(Answer: an address, but one that it is no longer legal for the program to access.)

(On a side note, your p_pointer parameter is unused. You might as well omit it from the function.)

The correct handling of memory in instances like yours, where you wish to dynamically allocate within a function and pass the allocated memory to calling code, is beyond the scope of what has been covered so far. In overview, the responsibility to clean up the memory (call delete) now rests with the calling code:

...DataType * ptr = GetPointer();// use ptrdelete ptr;

Naturally, this is error prone as it logically separates allocation from deallocation, placing them at different levels of scope. What if the calling code forgets to call delete? What if ptr get reassigned?

To combat this, there are patterns and stratagems in C++ that generally go by the name of auto pointers. What they do is wrap the raw pointer in a special type of object that then releases the memory the pointer references when it goes out of scope:

[advanced for this stage]

std::auto_ptr< DataType > ptr (GetPointer());// when ptr goes out of scope, the auto_ptr destructor will call delete on the memory internally held in a DataType pointer.

An even better stratagem is to have the allocating function wrap the memory and return an auto pointer, nullifying the possibility of a memory leak due to failure to deallocate. Note, however, that auto_ptr is a fairly basic auto pointer, and does not cleanly handle a number of cases such as copies and reassignment. The Boost Library provides a variety of policy based auto pointers that better handle such cases. Nevertheless:
std::auto_ptr< DataType > GetPointer()
{
std::auto_ptr< DataType > ptr = new DataType;

// set up

return ptr;
}

...

// in calling code:
std::auto_ptr< DataTpe > p = GetPointer();

// everything takes care of itself now
[/advanced]

Everything that you new, you must delete exactly once. Not once in the sense of lines of code that would do that deletion, but once in the sense of times that it happens when the program is run. Similarly, everything that you new[], you must delete[] exactly once. new and new[] are not the same, and you are required to pair them with the corresponding delete/delete[]. Because a pointer has no knowledge of whether it points at an element or at an array, you can't effectively write code that decides which version to use, so you should set things up such that for any pointer, it is known ahead of time whether it will point at a new thing or a new[] thing (or an existing thing). Keep in mind with pointers-to-existing thing, that if you have multiple pointers to the same thing, and you delete using one of them, the other pointers are now "dangling" - unusable until they are reset to point to something valid. If your design involves "sharing" objects, you have to be extremely careful, because there's no easy way to just "locate all pointers that point to an object and reset them".

The deletion should only happen once you are done with the pointed-at thing. In your function, you might need to delete the existing m_Pointer before doing the new, but you should not delete afterwards, because you are going to return a copy of the pointer. There's no sense in having your function return a pointer to something that you just deleted, because that pointer is already invalid.

Also, there is no need to assign to a temporary there; you can just say 'm_Pointer = new Whatever'.

Quote:
Let's retrace your steps: you allocate memory dynamically within the function, then set an external pointer to reference that piece of memory. And then you delete the memory, leaving the external pointer (m_pPointer, which is the return value of the function) pointing at... what?

I'm confused about this. Does this mean that in the following piece of code the delete on pTemp will free the memory allocated for pTemp? Eventhough pNew has a reference to the allocated memory?

DataType* pTemp = new DataType;DataType* pNew = pTemp;delete pTemp;

Oh, that makes sense. Now that I look back at my question I have no clue why I was confused in the first place :P

Quote: Original post by RinusMaximus

Quote:
Let's retrace your steps: you allocate memory dynamically within the function, then set an external pointer to reference that piece of memory. And then you delete the memory, leaving the external pointer (m_pPointer, which is the return value of the function) pointing at... what?

I'm confused about this. Does this mean that in the following piece of code the delete on pTemp will free the memory allocated for pTemp? Eventhough pNew has a reference to the allocated memory?

DataType* pTemp = new DataType;DataType* pNew = pTemp;delete pTemp;

Yes. 'delete' says "see this chunk of memory? I don't need it any more." The system does not try to account for other variables that might also point to it, because (a) it can't do so safely (it is possible, although a bad idea, to store pointer values into non-pointer types); (b) if it could keep track of such things ([google] "garbage collection"), you wouldn't need 'delete' at all. Remember, C++ is a low-level language. The burden for this sort of thing generally falls on the program.