Sign in to follow this  
localrob2

deleting pointers in C++

Recommended Posts

I was under the impression that anytime you use the keyword new when instantiating a class, you should use delete to deallocate the memory. I have an Application class that has pointers to another class, State. The Application is constructed and destructed correctly, but the State object is created but never destroyed. Is it automatically destroyed when the program is closed? If so, then shouldn't the destructor for the Application and State class get called? Here is some pseudocode of what I have.
Application* g_pGameApp;
Main()
g_pGameApp = new Application();
g_pGameApp->Init()
 {
  State* Menu = new State()
 }
g_pGameApp->Run()
 {
  //stuff happens each frame
 }
g_pGameApp->Shutdown()


If I try and delete the State in the Application Shutdown it crashes.

Share this post


Link to post
Share on other sites
You dont need to delete anything right before the program crashes, windows will take care of that. You only need to worry about new and delete when your dealing with memory being created and deleted at run-time.

"If so, then shouldn't the destructor for the Application and State class get called?"

-No, windows takes care of your program in RAM. It knows nothing of your code or classes, it will just free up the memory taken by your application.

Share this post


Link to post
Share on other sites
@crazyfool
Thanks for the quick replies. I guess my pseudo code was written poorly. The State pointer is a member of the application class. It is instantiated in the Init function, and then manipulated in Run().

Share this post


Link to post
Share on other sites
I imagine it's crashing because you are possibly trying to use state after deleting it. Why not just delete the State* in the destructor for the Application class, and delete the application class that you created at the end of the program, after you call it's shutdown method. You should really delete your pointers in the reverse order in which you created them, to prevent trying to use a dead pointer.

Share this post


Link to post
Share on other sites
Quote:
Original post by localrob2
I was under the impression that anytime you use the keyword new when instantiating a class, you should use delete to deallocate the memory.

This is absolutely almost always true. An exception might be, for example, in the case of a "smart" pointer that does the delete for you. Put the delete back in and run your application in the debugger to find out where and why it is crashing.

Share this post


Link to post
Share on other sites
Quote:
Original post by dpadam450
You dont need to delete anything right before the program crashes, windows will take care of that. You only need to worry about new and delete when your dealing with memory being created and deleted at run-time.

"If so, then shouldn't the destructor for the Application and State class get called?"

-No, windows takes care of your program in RAM. It knows nothing of your code or classes, it will just free up the memory taken by your application.


That's all well and good, but you need to remember that Windows will only free the memory. Destructors will not be called. Also, the behavior you're describing is platform dependent. Some operating systems will not free up memory for you.

Just because you can get away with it doesn't mean you should.


jfl.

Share this post


Link to post
Share on other sites
If you can, run the code in debug mode and try and find the line on which the crash occurs. It will help determine if you're deleting a garbage-pointer, deleting a null-pointer, or using a pointer post-deletion.

Quote:
Original post by jflanglois
That's all well and good, but you need to remember that Windows will only free the memory. Destructors will not be called. Also, the behavior you're describing is platform dependent. Some operating systems will not free up memory for you.
Just because you can get away with it doesn't mean you should.

Agreed.
The RAII paradigm lends itself very strongly to C++ development, and under this paradigm destructors do a lot more than just freeing up RAM.

Share this post


Link to post
Share on other sites
If you create an object at runtime using new it allocates a load of memory to store the object then calls the constructor. If you don't call delete when you are done the memory will not be freed and destructors wont be called. Most modern operating systems will clean up after you at the end of the program but that doesn't help in this situation:


void Class::DoSomething() {
for (int i = 0; i < 10; ++i) {
m_Object = new Object();
m_Object->DoSomethingElse()
}
}




That would create 10 new objects, and at the end of the function m_Object will only point to the last one. In this instance you will have NO way of deleting the other 9 objects you created. Therefore you just leaked 9 * sizeof(Object)s worth of memory.

This would be much better (still not great coz you wouldn't want to create objects on the heap for this but its just an example)


void Class::DoSomething() {
for (int i = 0; i < 10; ++i) {
m_Object = new Object();
m_Object->DoSomethingElse()
delete m_Object;
m_Object = 0;
}
}





You don't NEED to set m_Object to zero once deleted, but it does allow you the opportunity to check and see if it points somewhere:

if (m_Object != 0) {
//Object exists
}

Also if you use new you need to use the right version of delete:


Object *obj = new Object()

delete obj;

Object *obj = new Object[10]; //Creates 10 objects in consecutive memory
delete [] obj; //Deletes all 10 objects and calls their destructors



Luke.

Share this post


Link to post
Share on other sites
Alright I found the problem. The base class of State, in it's destructor, it was trying to close out a font object that was also closed out in the child state class. I moved the font_close() to a new shutdown function in State, insuring that it will only be called once, I also moved the deletes in the App class to the destructor.

One more question, if I have a pointer State* _pCurState, but I never instantiate it, do I need to delete that? For example,
State* _pMenuState = new State();
State*_pCurState = _pMenuState;
delete _pMenuState:

Do I need to delete _pCurState?

Thanks for all the replies.

Share this post


Link to post
Share on other sites
Quote:
Original post by localrob2
One more question, if I have a pointer State* _pCurState, but I never instantiate it, do I need to delete that? For example,
State* _pMenuState = new State();
State*_pCurState = _pMenuState;
delete _pMenuState:

Do I need to delete _pCurState?


No. For every 'new' you should have one 'delete'. You have one new here so you can only delete once.

Also, the use of identifiers beginning with an _ is considered bad practice since they are reserved for the implementation - though yours are not likely to cause problems.

Share this post


Link to post
Share on other sites
Quote:
Original post by Simian Man
Also, the use of identifiers beginning with an _ is considered bad practice since they are reserved for the implementation - though yours are not likely to cause problems.


I usually prefix those with m,g,etc depending on where it is. I left that off...I guess just because.
m_pCurState for a class data member, am I wrong?

Share this post


Link to post
Share on other sites
Quote:
Original post by localrob2
Quote:
Original post by Simian Man
Also, the use of identifiers beginning with an _ is considered bad practice since they are reserved for the implementation - though yours are not likely to cause problems.


I usually prefix those with m,g,etc depending on where it is. I left that off...I guess just because.
m_pCurState for a class data member, am I wrong?

Under (systems)Hungarian-notation "m_" would denote a "member variable" and "p" denotes "this is a pointer".

Some (lazy) people like to drop the "m" so it becomes "_pCurState", but as Simian Ma said, this is not standards compliant.
Other people (who like standards, but dont like hungarian) would instead use "CurState_" (the "_" on the end denotes a member variable).

There is no right way, just pick a convention you like and stick with it ;)

Share this post


Link to post
Share on other sites
Quote:
Original post by Simian Man
Also, the use of identifiers beginning with an _ is considered bad practice since they are reserved for the implementation - though yours are not likely to cause problems.


Although that's good advice, not all identifiers starting with an underscore are reserved. I quote:

Quote:
Working Draft, Standard for Programming Language C++ (c2006/N2134)
17.4.3.1.2 Global names [global.names]
Certain sets of names and function signatures are always reserved to the implementation:
— Each name that contains a double underscore __ or begins with an underscore followed by an uppercase letter (2.11) is reserved to the implementation for any use.
— Each name that begins with an underscore is reserved to the implementation for use as a name in the global* namespace.

* Such names are also reserved in namespace ::std (17.4.3.1)

Share this post


Link to post
Share on other sites
As a rule of thumb: Whenever you are about to use 'new', make sure you can explain clearly why it is needed. Many people get in a habit of dynamically allocating things all over - and then having problems sorting out the problem of when and where to deallocate them - when it is completely unnecessary.

The other features of C++ that will make your life easier are constructors and destructors. These allow you to implement the powerful idiom called RAII: Resource Acquisition Is Initialization. What it means is that you use the constructor (where the object is initialized) of an object to do your "resource acquisition" (typically, the "resource" is memory - i.e., dynamic allocation of something - but it could be a file handle - via an fstream object - or a network socket or something else). More importantly, though, is the unnamed, implicit part of the pattern - in the *de*structor of the object, you do the deallocation. The net result is that the resource's lifetime is tied to the containing object, and automatically managed that way.

Don't write Init() and Shutdown() member functions for an object. Use the constructor(s) and destructor instead. They are called automatically; that means (a) you can't forget to call them and (b) you don't have to do any extra work.

Let's suppose our Application class has a State* member, which points to its "current state". We might have derived classes of State that represent the main menu, etc. - all the states of the game. Each of these can "run" - I will spell that as "operator()" ;) - and when it's done running, will dynamically allocate the "next" state - another powerful design pattern, called "run and return successor" - an elaboration upon the State pattern that you already seem to be using.

Since everything that ever gets assigned to our 'current state' is dynamically allocated (and here, we can explicitly state the reason: the states are polymorphic, so we can't hold them by value in the Application because of "object slicing"), we have to delete for every new. The way we'll do this is:

- In the constructor, we new the initial state.
- Every time we invoke the current state, we new the next state (indirectly), and delete the current state (within the Application code).
- In the destructor, we delete the final state.

It is fairly easy to look over this structure and verify that every newed thing gets deleted exactly once. So, we proceed, with something like:


// And please do note several other minor things that I do differently - naming,
// scoping of the Application instance etc.

class Application {
State* current;
// etc.

// An Application instance should never be copied or assigned, so we
// can do the following to prevent that:
Application(const Application&); // DO NOT IMPLEMENT THESE.
Application& operator=(const Application&);

public:
Application() : current(new StartMenu()) {} // etc.
~Application() {
delete current;
// and anything else that needs to happen at "shutdown".
}

// Invoke the current state, and communicate whether the game is still running
bool operator()() {
// This is "dereference the pointer to get the state, and call its
// operator(), passing it a reference to myself as an argument."
State* next = (*current)(*this);
delete current;
current = next;
return !current.isFinal();
// Another way is to have the State operator() return NULL if the game
// should end. We check for null-ness here, and then there is no longer
// a need for a destructor, since the NULL doesn't need deleting.
}
};

int main() {
// We don't need to dynamically allocate the *application* instance.
Application game; // "init" work automatically done.
while (game()); // "stuff happens each frame"; the loop is empty, as
// all the work is done via the Application::operator().

// "shutdown" work automatically done at the end of the function.
}

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this