Self-Deleting Objects with Inheritance

[FrOzeN89]

Hi there,

I'm writing a bunch of classes like Form, Button, etc. All these are derived from a base Object class. I want to make these objects auto-delete, so I only have to declare the new keyword, and the program will clean them when necessary. I understand the implications of dereferencing, I just have a couple of questions about my code.

I through this run-down example together:

#include <Windows.h>
#include <map>

class Object
{
public:
	Object() {};
	~Object() {};

	void *operator new(size_t size);
	void operator delete(void *ptr);
	void Dispose();
	
private:
	static std::map<Object *, bool> m_Dynamic;
};

void *Object::operator new(size_t size)
{
	Object *newObj = (Object *)malloc(size);
	m_Dynamic.insert(std::pair<Object *, bool>(newObj, true));
	return newObj;
};

void Object::operator delete(void *ptr)
{
	m_Dynamic.erase((Object *)ptr);
	free(ptr);
};

void Object::Dispose()
{
	if (m_Dynamic[this])
		delete this;
};

std::map<Object *, bool> Object::m_Dynamic;		// Initialise static variable

class Circle: public Object
{
public:
	Circle() {};
	~Circle() {};

	void Destroy();

private:
	int m_Unused[10];
};

void Circle::Destroy()
{
	Object::Dispose();
}

int WINAPI WinMain(HINSTANCE hThisInstance,	HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow)
{
	Circle *obj = new Circle();

	obj->Dispose();	// indirectly delete object

	return 0;
};

I have added a static variable of std::map type called m_Dynamic. I have overloaded the new and delete keywords (within the Object class) to keep the m_Dynamic map up-to-date. This prevents objects created on the stack from being deleted.

The object itself can then be deleted via the base Dispose() method. Or methods such as Destroy() can be added in derived classes and call upon Object::Dispose() themselves. This can be overloaded, etc, but eventually will be removed from the public view. I only have it here for testing.

From what I can tell everything "works", though I'm uncertain that the object is being correctly deleted.

Though, my main concern is that when a derived class such as Circle calls Dispose() which in turns fires delete this. Does it free() the sizeof(Object) or does it correctly free() the sizeof(Circle)?

Are there any other things I should be vary of? I only just started playing with new/delete overloading yesterday.

Thanks!

[Paul McKenzie]

Hi there,

I'm writing a bunch of classes like Form, Button, etc. All these are derived from a base Object class. I want to make these objects auto-delete, so I only have to declare the new keyword, and the program will clean them when necessary.

Why not just use a smart pointer such as std::shared_ptr or std::unique_ptr?

Regards,

Paul McKenzie

[Paul McKenzie]

I have added a static variable of std::map type called m_Dynamic.

Which means that your code cannot be run in anything but a single-threaded program unless you add synchronization.

I have overloaded the new and delete keywords (within the Object class) to keep the m_Dynamic map up-to-date. This prevents objects created on the stack from being deleted.

So what is preventing a user from leisurely just calling the global :elete on their own? How does your map get updated in that scenario? Quick answer -- it won't be updated, and your map now contains an invalid pointer.

The object itself can then be deleted via the base Dispose() method. Or methods such as Destroy() can be added in derived classes and call upon Object::Dispose() themselves. This can be overloaded, etc, but eventually will be removed from the public view. I only have it here for testing.

The other major flaw is that your base class's destructor is not virtual.

From what I can tell everything "works", though I'm uncertain that the object is being correctly deleted.

See above.

1) No virtual destructor in base class (causing undefined behaviour to occur if a derived class is deleted via a base class pointer).
2) No way to prevent user from calling :elete (causing your map to store invalid pointers).
3) Your class is not thread-safe (causing your code to blow up if multiple threads happen to update the map at the same time).

I only just started playing with new/delete overloading yesterday.

And in a real-world situation, overloading operator new/delete is rarely done by user programs, if ever at all. That's why using things such as smart pointers are better alternatives to what you've presented. A smart pointer uses RAII, which means that an object will destroy itself correctly, or if need be, a "reset()" member function can be used to destroy instances.

Regards,

Paul McKenzie

[2kaud]

From what I can tell everything "works", though I'm uncertain that the object is being correctly deleted.

If you're uncertain about anything, then you don't know that everything 'works' and are just guessing/hoping. Through logical and systematical testing you either determine that the program doesn't work or gain a high level of confidence that the program 'works' as expected.

[FrOzeN89]

First, before I get shot to pieces in regards to my "design pattern". Please understand all this is just me learning C++. I've only used it for less than 2 months and I accept that all my code for quite some time is going to be _designed_ rather poorly until I grasp a better understanding and hit up things like Code Complete, etc. Right now my project is making a small Win32 wrapper to create windows, controls, etc, and handle them in an event-driven manner.

Why not just use a smart pointer such as std::shared_ptr or std::unique_ptr?

I am not familiar with these as yet.

I'm also not sure how the pointer would determine to delete the object appropriately (unless it was a decent GC maybe). Effectively inside the object, I want to delete the class when WM_DESTROY is called, along with other code that will clean up other things as well.

My code looks similar to this (stripped down):

#include <windows.h>
#include "frzn_controls.h"

EVENT fMain_Create;
EVENT btn1_Click;
EVENT btn2_Click;

int WINAPI WinMain(HINSTANCE hThisInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow)
{
	FormSetup fs(L"Main Window", 860, 560);
	fs.Events.Create = fMain_Create;
	
	Form *fMain = new Form(fs);
	fMain->Show();

	// Run Windows Loop
	int MessageResult;
	App::RunMessageLoop(MessageResult);

	delete fMain; // I want this to be called from inside the class when WM_DESTROY is parsed to fMain->WndProc.
		// The Button objects that were created in the fMain_Create will also have their pointers automatically associated with the fMain object, it will Dispose of them when it is deconstructing.

	return MessageResult;
}

void fMain_Create(Object *sender, EventArgs *e)
{
	// Create Button 1
	ButtonSetup bs(L"First button!", 140, 25, 10, 10);
	bs.Parent = *(Form *)sender; 	// *[Form] is HWND overloaded to return it's hWnd
	bs.Events.Click = btn1_Click;
	Button *btn = new Button(bs);

	// Create Button 2
	bs.Top = 45;
	bs.Title = L"Second Button!";
	bs.Events.Click = btn2_Click;
	Button *btn2 = new Button(bs);
}

void btn1_Click(Object *sender, EventArgs *e)
{
	MessageBox(*(Form *)sender, L"Button 1 clicked!", NULL, 0);
};

void btn2_Click(Object *sender, EventArgs *e)
{
	MessageBox(*(Form *)sender, L"Button 2 clicked!", NULL, 0);
};

As you can see from this code, manually deleting objects is a bit tedious and it is much easier to have them suicide themselves. For this example alone I would have to add another event after Form.Close just so I could delete the 2 Button objects. I would also have to tie them into a <map> associated with the Form object which I could then expose via *sender just to avoid global variable usage.

Which means that your code cannot be run in anything but a single-threaded program unless you add synchronization.

I have to use a static or global variable. I initially made m_Dynamic a boolean which was modified upon the new call. But straight after the constructor fires (oops.. durr) and the initialiser overwrites any variables I try pre-settings within new. Threading is something I have not yet accounted for as I've never looked into it outside the basic concepts. Though, going over other code I've seen, I would imagine I could simply Add and Retrieve pointers in a <map> by using a simple CriticalSectionEnter/Leave wrapper?

So what is preventing a user from leisurely just calling the global :elete on their own? How does your map get updated in that scenario? Quick answer -- it won't be updated, and your map now contains an invalid pointer.

I could easily move the code from Dispose() to delete and have Dispose just call delete. Or simply overload delete so it does nothing.

The other major flaw is that your base class's destructor is not virtual.
See above.

1) No virtual destructor in base class (causing undefined behaviour to occur if a derived class is deleted via a base class pointer).

Ah ok, didn't know that. I have applied the change.

And in a real-world situation, overloading operator new/delete is rarely done by user programs, if ever at all. That's why using things such as smart pointers are better alternatives to what you've presented. A smart pointer uses RAII, which means that an object will destroy itself correctly, or if need be, a "reset()" member function can be used to destroy instances.

Briefly looking at my code example using Forms. Would overloading the new operator to manage these objects be a suitable scenario?

If you're uncertain about anything, then you don't know that everything 'works' and are just guessing/hoping. Through logical and systematical testing you either determine that the program doesn't work or gain a high level of confidence that the program 'works' as expected.

I absolutely agree. I wrote works in quotes to imply "AFAIK it works", though, I knew my knowledge was limited and it might not actually work in the way I was hoping to achieve.

[EDIT]

Another note. I tried using sizeof(ptr) and have a MessageBox display the size of the this pointer. Inside the base class it returns size 4, though, it returns size 44 in the derived Circle. Is free() correctly using the right size and sizeof() is simply assuming because it's compile-time. Or if I use "delete this" or "free(this)" inside a base object is will only free the amount of space the object takes, not accounting for it's parents?

[EDIT 2] Regarding the first edit, how would I go about Debugging something like this myself? Problematic memory code usually will always "run" and likely lead to undefined behaviour or crashing further down I would imagine..

[laserlight]

I'm also not sure how the pointer would determine to delete the object appropriately (unless it was a decent GC maybe).

Reference counting, in the case of a typical implementation of std::shared_ptr.

As you can see from this code, manually deleting objects is a bit tedious and it is much easier to have them suicide themselves.

The concept used in C++ is Resource Acquisition is Initialisation (RAII), which is actually a technique that associates the lifetime of a resource with the lifetime of an object. So, if you have a smart pointer (the RAII object) to some dynamically allocated object (the RAII resource), then when the smart pointer goes out of scope, it will destroy the object if that is appropriate.

With such an approach, the only time when you will explicitly write a delete or delete[] somewhere is when you are implementing the RAII mechanism itself, e.g., if you are implementing a smart pointer. You might write new or new[], but then the result would be immediately be handed off to a smart pointer. Or, you might use a container that manages the memory for you via RAII.

[OReubens]

Your solution doesn't "automatically" delete all the objects, or rather, maybe it does at the very end of your program, but for all intents and purposes that still means that as long as your program is running you have a memory leak, because objects get newed, but aren't getting deleted unless something triggered the actual delete. Things might be better if for example you had an object container as part of a dialog/window/form and upon deleting the dialog/window/form all it's child objects died with it. But that doesn't seem to be what you're aiming at.


You also didn't implement the array versions of your class new/delete.


using malloc/free in a C++ app is still "bad form", even if you use it to implement a new/delete (and even though the C++ runtime probably ends up doing it that way). Use global new/delete instead.

[Paul McKenzie]

First, before I get shot to pieces in regards to my "design pattern". Please understand all this is just me learning C++. I've only used it for less than 2 months and I accept that all my code for quite some time is going to be _designed_ rather poorly until I grasp a better understanding and hit up things like Code Complete, etc. Right now my project is making a small Win32 wrapper to create windows, controls, etc, and handle them in an event-driven manner.

Well, the thing you should do is to see what is offered to you by the C++ library at a higher-level of abstraction.

I honestly don't think getting into the weeds of overloading new and delete is the way to go here. Unless you're creating some sort of memory pool manager, or attempting to overload new for debugging purposes, I see no reason to go down the road you have gone down. Even if you were creating a memory pool manager, your experience would have been at a level where you would practically never need to ask a question on a C++ forum.

I am not familiar with these as yet.

I'm also not sure how the pointer would determine to delete the object appropriately (unless it was a decent GC maybe).

C++ has something called smart pointers. These pointers do know when to delete the object, and even gives you the opportunity to delete the object yourself.

The first rendition of a smart pointer offered by the C++ library was auto_pointer. This was a decent start, but this class was flawed due to it not having correct copy semantics (i.e. you couldn't store auto_pointer's in a container). Later on, this was addressed by adding std::shared_ptr and std::unique_ptr.

All of these smart pointers use, as laserlight pointed out, a technique called RAII. What this means is that instead of writing code like this:

void foo()
{
    SomeResource* p = new SomeResource;
    if ( whatever )
    {
         delete p;
         return;
    }
    //... more code
    if ( whatever_2 )
    {
         delete p;
         return;
   }
   //... some more code 
   //...
   delete p;
}

What if you had 2, 3, 4, or more return points? With your approach, you would have to cover all of those return points with a "delete p" statement. What if you miss a return point? What if a new coder comes along, adds a return point, and doesn't realize that a "delete p" needs to be done? What if an exception is thrown somewhere in the foo() function, and the exception causes an exit from the function, therefore not giving the code the chance of hitting any of those return points?

All of these problems are solved by using classes that have the RAII paradigm built in.

#include <memory>

void foo()
{
    std::shared_ptr<SomeResource> p = std::shared_ptr<SomeResource>(new SomeResource);
    if ( whatever )
    {
         return;
    }
    //... more code
    if ( whatever_2 )
    {
         return;
   }
   //... some more code 
   //...
}

So what happened to the calls to "delete p"? They're not needed. The reason is that shard_ptr will automatically call delete on the pointer when the pointer goes out of scope (to be more exact, shared_ptr will delete on the pointer when no other objects are "sharing" the pointer, i.e. the reference count reaches 0).

If you want a real-life example sitting right there at your compiler, look at the ifstream and ofstream classes. You open a file, and note you never need to call close() (you can if you want to, but why is it not mandatory that close() is called?). For the same reason that delete didn't need to be called on the smart pointers -- RAII. The stream objects know to close the handle when they go out of scope.

Effectively inside the object, I want to delete the class when WM_DESTROY is called, along with other code that will clean up other things as well.

Since a smart pointer will clean itself up, then maybe you don't need to manually clean up anything.

Regards,

Paul McKenzie

[nuzzle]

What if you had 2, 3, 4, or more return points?

That would be bad programming in its own right.

[laserlight]

That would be bad programming in its own right.

Not necessarily, though the "return points" could well just be points where an exception might be thrown or propagated.

[nuzzle]

I'm also not sure how the pointer would determine to delete the object appropriately (unless it was a decent GC maybe).

Reference counting smart pointers can be considered GC light, and std::shared_ptr is the standard C++ way of accomplishing "self-deleting" objects.

Reference counting has one major drawback though and that is they cannot handle circular referencing. It will result in a memory leak.

[nuzzle]

Not necessarily, though the "return points" could well just be points where an exception might be thrown or propagated.

A thrown exception is not a return point.

Multiple return points increases code complexity. Therefore they're error prone and should be avoided (although I sometimes use them myself in idiomatic cases such as entry guards and loop escaping). As used in the Paul McKenzie example I referred to they're just bad programming. When you encounter code like that you don't introduce a smart pointer to fix it, you kick azzes.

[laserlight]

A thrown exception is not a return point.

Agreed, hence the quotes. However, with respect to the topic at hand, the result is the same: if you don't use RAII, then you have to consider that control may leave the current scope without the resource having been freed.

Multiple return points increases code complexity. Therefore they're error prone and should be avoided

Not always.

I sometimes use them myself in idiomatic cases such as entry guards and loop escaping

Same here, and indeed these are cases where I don't think that they increase code complexity, and their use is fine. Another case that comes to mind is when there is say, an if/else statement at the end of a function, and both branches have a return statement. The logic would still be plain to see.

When you encounter code like that you don't introduce a smart pointer to fix it, you kick azzes.

If you don't have the time to do a better refactoring, introducing a smart pointer is better than trying to "kick azzes", whatever that means.

[Codeplug]

>> Multiple return points increases code complexity.

In "C++ Coding Standards", #20 is "Avoid long functions. Avoid deep nesting." Enforcing a single-return rule can result in deep nesting. And if the function is short to begin with, then the reader is less likely to be thrown off by multiple return points.

"If your function is long enough that multiple returns are a problem, it's too long."

gg

[FrOzeN89]

Reference counting, in the case of a typical implementation of std::shared_ptr.

I just spent a while looking up as much RAII info as I could find, and then implemented an example of the std::shared_ptr. Though, this failed because the class isn't ready to be destroyed when the reference count is 0. Effectively, once the Form is created, for most cases, the only external reference pointer to the form is passed via WNDCLASSEX.lpfnWndProc parameter. So interaction to the Form is via it's WndProc method. It's not until this method receives WM_DESTROY does the object need to be destroyed. I'm not sure a pointer class (except where I mentioned maybe a good GC) could determine such a case, and therefore I figured I have to implement my own destruction of the object.

A very common example usage of my class would be a button that brings up the Settings form. i.e:

void btn_Settings(Object *sender, EventArgs *e)
{
	FormSetup fs(L"Settings", 600, 400);
	fs.Events.Create = fSettings_Create;

	Form *fSettings = new Form(fs);
	
	// After the next line Show() this function finishes and the shared_ptr goes out of scope which deletes my object.
	// As soon as I move the mouse over the Settings window my program crashes because of an Access Violation when Windows tries calling Form::WndProc.

	// std::shared_ptr<Form> fSettings(new Form(fs));	// Deletes Form too soon!

	fSettings->Show();
};

With such an approach, the only time when you will explicitly write a delete or delete[] somewhere is when you are implementing the RAII mechanism itself, e.g., if you are implementing a smart pointer. You might write new or new[], but then the result would be immediately be handed off to a smart pointer. Or, you might use a container that manages the memory for you via RAII.

Given my objects are tied to Windows callbacks, maybe creating my own RAII method is the best case scenario?

Otherwise the option might be I create a global container and add the smart pointer on 'new', and then remove it when needed (i.e WM_DESTROY). This way I would not be expliciting be playing with new/delete, but indirectly doing so by controlling the reference count?

Your solution doesn't "automatically" delete all the objects, or rather, maybe it does at the very end of your program, but for all intents and purposes that still means that as long as your program is running you have a memory leak, because objects get newed, but aren't getting deleted unless something triggered the actual delete. Things might be better if for example you had an object container as part of a dialog/window/form and upon deleting the dialog/window/form all it's child objects died with it. But that doesn't seem to be what you're aiming at.

I'm not sure what you're getting at? I'm trying to discuss a means of which I can appropriately have the object self-delete itself. I know I could expose methods and call delete, but it's redundant. Right now, their is no clear-method I've seen to delete the Form object. It only works in WinMain() because that remains in scope throughout the entirety of the program. Even though, if I made it load a splash window or something, I would like to have that object delete sooner that later.

You also didn't implement the array versions of your class new/delete.

Thanks, I'll definitely add this if the final solution still uses new/delete overloading.

using malloc/free in a C++ app is still "bad form", even if you use it to implement a new/delete (and even though the C++ runtime probably ends up doing it that way). Use global new/delete instead.

The first results on google demonstrated this way. I just assumed using new/delete inside themselves would create a recursion issue. I'll look back to using new/delete as I'm trying to use C++ style everywhere feasible.

-----
Probably something worth noting which was confusing me quite a bit about "how smart pointers just knew" was that I'm actually doing something particularly out of the ordinary which I didn't initially realise had to do with my new/delete question. I used the ATL's thunk method to provide a (this) pointer to WNDCLASSEX for WndProc. So technically, my reference count of 1 to my objects is tucked away very deep.

Is there a common smart pointer out their which your class requires to implement a specific Interface to control it's disposal? That way it would still be reasonably RAII-style etiquette?

[EDIT] After searching more, it seems using a shared_ptr to manage a COM object matches up to be a reasonable solution to my auto-delete concept?