Jump to content
  • Advertisement

Archived

This topic is now archived and is closed to further replies.

sirSolarius

Code structuring problems

This topic is 5220 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Alright, I have a little dilemma in my code structure. I have a base MemObj class with a reference counter, and a smart pointer MemPtr to support it. Running these is my MemoryManager class, which deletes MemObj''s that lack references. Now, my issue is that I cannot think of a good way to allow my MemoryManager to clean up MemObj''s that get allocated in the main loop. Here is my example:
int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow)
{
	////////////////// Enable memory leak detection ///////////

	#ifdef _DEBUG
		_CrtDumpMemoryLeaks();
		_CrtSetDbgFlag ( _CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF );
	#endif
	//////////////////////////////////////////////////////////


	// Start by creating my class and window

	WindowManager *wnd=WindowManager::getInstance();
	wnd->CreateClass("Kutatas", WindowsMsgHandler, hInstance);
	wnd->SpawnWindow("Kutatas Engine", 1024, 768);
	wnd->SetAsGLWindow();
	wnd->setFullScreen(true);

	InputManager *mainInput=InputManager::getInstance();
	MemPtr<InputKeymap> keymap=new InputKeymap();
	MemPtr<Functor> escape=new Functor(&ExitProgram);
	keymap->Assign(DIK_ESCAPE, escape);

	MemoryManager *memManage=MemoryManager::getInstance();

	g_state=RUNNING;

	MSG msg;		// Message for recieving Windows messages


	while(g_state == RUNNING)
	{
		if (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE))
		{
			TranslateMessage(&msg);
			DispatchMessage(&msg);
		}

		mainInput->Update();
		memManage->checkMemory();
		
		keymap->Update();

		if(g_state==PAUSED)
			g_state=RUNNING;
	}

	MemoryManager::getInstance()->Destroy();
	return 1;	
}
Both keymap and escape never get deleted here, because they only deconstruct when WinMain ends (and therefore when MemoryManager can''t check them). How can I fix this problem?

Share this post


Link to post
Share on other sites
Advertisement
First thing - it depends upon your implementation of the singleton for your memory manager, but it looks like the memory manager doesn''t get created until after keymap and escape are created. I know this may not be the case, but it''s difficult to tell. WHcih leads on to the second point: how to solve your problem.

By creating the memory manager first, you can then encompass the rest of the code in a local scope so that the MemPtr''s go out of scope i.e.



int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow)
{
////////////////// Enable memory leak detection ///////////

#ifdef _DEBUG
_CrtDumpMemoryLeaks();
_CrtSetDbgFlag ( _CRTDBG_ALLOC_MEM_DF | _CRTDBG_LEAK_CHECK_DF );
#endif
//////////////////////////////////////////////////////////


MemoryManager *memManage=MemoryManager::getInstance();

{
//Main Code here

}

memManage->Destroy();
}

Share this post


Link to post
Share on other sites
quote:
Original post by jamessharpe
First thing - it depends upon your implementation of the singleton for your memory manager, but it looks like the memory manager doesn''t get created until after keymap and escape are created. I know this may not be the case, but it''s difficult to tell. WHcih leads on to the second point: how to solve your problem.

By creating the memory manager first, you can then encompass the rest of the code in a local scope so that the MemPtr''s go out of scope i.e.



No, the singleton avoids that because it is generated when accessed for the first time by ANYTHING, so even if it doesn''t exist when an object calls markForDeletion, it is created and the new object is marked.

The artificial scope trick worked, btw, but I''m still leaking memory somewhere...

Share this post


Link to post
Share on other sites
quote:
Original post by sirSolarius

No, the singleton avoids that because it is generated when accessed for the first time by ANYTHING, so even if it doesn''t exist when an object calls markForDeletion, it is created and the new object is marked.



The thing is that this is a common abuse of the singleton design pattern - making it into a global variable accessor to avoid the use of the oh so bad global variable(albeit with a little more control over construction time).

The proper use of a singleton is where you use the singleton class to provide a pointer to the instance, and then you pass this pointer around. What this does is ensures that their will only ever be one instance and you have complete control over its life. In your code if another call to Singleton::getInstance() is made after you call destroy, then the singleton class will happily make the singleton again, but it won''t be destroyed. This is the kind of snare can cause havok with your code, when you expect it to have been destroyed but it has been created again.

Btw when the CRT reports the leaks if the allocation number is the same on multiple runs, then you can use the function _CrtSetBreakAlloc to force a breakpoint when the allocaiton occurs, enabling you to find the culprit of the leak...

James

Share this post


Link to post
Share on other sites
Ok, more annoying weirdness. I set a breakpoint for keymap''s deconstructor, and it never calls. This is what''s causing the memory leak, as the escape functor is never deconstructed.

Here is the keymap code:

// InputKeymap.cpp: implementation of the InputKeymap class.

//

//////////////////////////////////////////////////////////////////////


#include "InputKeymap.h"

//////////////////////////////////////////////////////////////////////

// Construction/Destruction

//////////////////////////////////////////////////////////////////////


InputKeymap::InputKeymap()
{
}

InputKeymap::~InputKeymap()
{
Erase();
}

//////////////////////////////////////////////////////////////////////////////////////////

// Name: Assign

// Desc: Maps a key to a function

// Notes:

// ToFix:

//////////////////////////////////////////////////////////////////////////////////////////

void InputKeymap::Assign(int key, MemPtr<Functor> &f)
{
m_keymap[key]=f;
}

//////////////////////////////////////////////////////////////////////////////////////////

// Name: Unassign

// Desc: Unmaps a key completely

// Notes:

// ToFix:

//////////////////////////////////////////////////////////////////////////////////////////

void InputKeymap::Unassign(int key)
{
m_keymap[key]=0;
m_keymap.erase(key);
}

//////////////////////////////////////////////////////////////////////////////////////////

// Name: Erase

// Desc: Erases the keymap and deletes its functors

// Notes:

// ToFix:

//////////////////////////////////////////////////////////////////////////////////////////

void InputKeymap::Erase()
{
std::map<int, MemPtr<Functor> >::iterator iter=m_keymap.begin(), e=m_keymap.end();
while(iter != e)
{
(iter->second)=0; // set the functor to 0 for memory management

++iter;
}

m_keymap.clear();
}

//////////////////////////////////////////////////////////////////////////////////////////

// Name: Update

// Desc: Checks the input manager for pressed keys, and activates functions if pressed

// Notes:

// ToFix:

//////////////////////////////////////////////////////////////////////////////////////////

void InputKeymap::Update()
{
InputManager *p=InputManager::getInstance();
std::map<int, MemPtr<Functor> >::iterator iter=m_keymap.begin(), e=m_keymap.end();
while(iter != e)
{
if(p->keyDown(iter->first)) // if key is pressed

(*(iter->second))(); // activate the function

++iter; // then advance to the next key

}
}


And then my MemObj and MemPtr code:

#ifndef _ERIK_MEMOBJ_H_DEFINED
#define _ERIK_MEMOBJ_H_DEFINED

#include <assert.h>
#include <list>

#include "MemoryManager.h" // include the memory management class


/////////////////////////////////////////////////////////////////////////////

// MEMOBJ.H

/////////////////////////////////////////////////////////////////////////////

//

// AUTHOR: ERIK GOLDMAN

// DATE: 6-3-04

// DESC: The base memory-managed object class

// NOTES:

// TOFIX:

/////////////////////////////////////////////////////////////////////////////


class MemObj
{
protected:
int m_refs;

public:
MemObj()
:m_refs(0)
{
}

void attach()
{
++m_refs;
}
void release()
{
--m_refs;
if(m_refs==0) // nothing points to the object

{
MemoryManager::getInstance()->markForDeletion(this);
}
}
};


/////////////////////////////////////////////////////////////////////////////

// MEMPTR

/////////////////////////////////////////////////////////////////////////////

//

// AUTHOR: ERIK GOLDMAN

// DATE: 6-3-04

// DESC: A smart pointer to a memory-managed object

// NOTES:

// TOFIX:

/////////////////////////////////////////////////////////////////////////////


template <class T>
class MemPtr
{
protected:
T *obj;

public:
MemPtr()
:obj(0)
{
}

MemPtr(const MemPtr<T> &p)
:obj(p.obj)
{
if(obj)
obj->attach();
}

MemPtr(T *p)
:obj(p)
{
if(obj)
obj->attach();
}

~MemPtr()
{
if(obj)
obj->release();
obj=0;
}

inline MemPtr &operator=(T *p)
{
if(obj)
obj->release();
obj=p;
if(obj)
obj->attach();
return *this;
}

inline MemPtr &operator=(const MemPtr &p)
{
if(obj)
obj->release();
obj=p.obj;
if(obj)
obj->attach();
return *this;
}

inline T& operator*() const
{
assert(obj!=0);
return *obj;
}

inline T *operator->() const
{
assert(obj!=0);
return obj;
}

inline bool operator==(const MemPtr<T> &p)
{
return(p.obj == obj);
}

inline bool operator ==(const T *p)
{
return(obj == p);
}

inline bool operator !()
{
return (obj == 0);
}

inline bool isValid()
{
return (obj != 0);
}
};

#endif

Share this post


Link to post
Share on other sites
I''m guessing checkmemory is where the objects are deleted? Are you calling this within the destroy function?

Just some minor points about the MemPtr code:

You''re missing some const''s on the operators == , !, isValid

The assignment operator should check for self assignment - consider this code fragment:



MemPtr<Object> o = new Object;
o = o;
memManage->checkMemory();
o->SomeFunction(); // BOOM! Access violation



Consider what happens: o has a refcount of 1. = is called and sets refCount to 0, marking the object for deletion. We then reset the count to 1, but it is on the memory managers list for deletion, so next time we try to access it - access violation.
Ok, so you''re not going to do anything stupid like in my example, but it can happen with more complex code, and would be a very difficult bug to track.

Share this post


Link to post
Share on other sites

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!