Memory leak?

Started by
8 comments, last by thejoshinator 19 years, 2 months ago
Hey guys, This is my first attempt at a copy constructor, but I'm not sure how exactly to go about copying the pointers. For example, I know I have to create a new HDC via the 'new' pointer, but do I set the new HDC equal to the hdc of the object being copied, or do I create a HDC that is compatible with it? Here's the code:
CMap::CMap(CMap &map)
{
	// This function is the copy constructor for the CMap class

	int index = 0; // looping variable

	// copy the shallow members
	this->map_height = map.map_height;
	this->map_width = map.map_width;
	this->scale = map.scale;
	this->scrn_height = map.scrn_height;
	this->scrn_width = map.scrn_width;
	this->start_tile = map.start_tile;
	strcpy(this->mapname, map.mapname);
	strcpy(this->tileset, map.tileset);

	// Make new bmps and hdcs
	this->hdc = *new HDC;
	this->bmp = *new HBITMAP;
	this->old_bmp = *new HBITMAP;

	// Copy the deep members
	this->hdc = CreateCompatibleDC(map.hdc); // is this right? or does it need to be "this->hdc = map.hdc"?
	this->bmp = map.bmp; // Same question here?
	this->old_bmp = map.old_bmp;
} // end CMap(CMap &map);

Thanks, The Joshinator
Advertisement
If you have the desire and need for the most accurate answer, download MMGR and add it to your project. Follow the directions in the .h and .cpp file which are #include the mmgr.h after system header files but before yours so it will compile correctly. Simply run once and look at the log generated. It will tell you if you have any memory leaks. I can personally vouch for this free code. It is the best thing I have ever seen. Not only that, Fluid Studios has some more great code, such as a logger for use.

Now if you do verify that you have memory leaks, someone else will have to exaplain the whole logics behind it. I am not that well versed with the whole Win32 drawing contexts. Hope this helps.

- Drew
I think you're just creating a new pointer to point to the original data. Try memcpy and use some profiler to check for leakage like the above poster suggest.

Also try creat an instance of the map and then copy it then delete the original and see what happen to the copy data.
You dont' need to create a new pointer to an hdc, just using createcompatableDC will work.

You should also modify your decalaration to take a const CMap& not a CMap&. Same for the Bitmaps, generally a deep copy cstr would create a clone of each object not another reference to each object. If all you need is a reference then a shallow copy cstr should work as none of your types is reference counted.

Cheers
Chris
CheersChris
Quote:...generally a deep copy cstr would create a clone of each object not another reference to each object.

That is what I want to do - create a deep copy with clones instead of references. How do i clone a HBITMAP, data and all?
You should never have a '*' in front of new like that. Not only are you not ever storing the pointer ANYWHERE, but since the fields themselves are not pointers to begin with then there would never be any need to allocate at all, so yes you definately have 3 leaks there.
You most certainly do mean to have pointers however, and want to allocate new new bitmaps etc and copy memory from one to the other.

Can you show us the declaration of CMap please?
"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms
Here it is:
// CMap classclass CMap{	public:		// public functions		CMap();			 // Constructor: Sets everything to 0 or NULL		CMap(const CMap &map); // Copy Constructor: makes deep copies of a CMap object		void Set_Mapname(char *name);									  // Set the mapname		void Set_Tileset(char *filename);								  // The tileset to use		void Set_Start_Tile(int index)		 {start_tile = index;}		  // Set the starting tile		void Set_Map_Width(int width)		 {map_width = width+2;}		  // Set the map width		void Set_Map_Height(int height)		 {map_height = height+2;}	  // Set the map height		void Set_Screen_Width(int width)	 {scrn_width = width+2;}		  // Set the screen width		void Set_Screen_Height(int height)	 {scrn_height = height+2;}	  // Set the screen height		void Set_Tile_Width(int width)		 {tile_width = width;}		  // Set the tile width		void Set_Tile_Height(int height)	 {tile_height = height;}		  // Set the tile height		void Set_Scale(float percent_change) {scale = percent_change/100;} // Set the scale of the map		char *Get_Mapname()	  {return mapname;}		// Returns the mapname		int Get_Tile_Width()  {return tile_width;}	// Returns the width of the tiles		int Get_Tile_Height() {return tile_height;} // Returns the height of the tiles		int Get_Map_Width()   {return map_width;}	// Returns the width of the map in tiles		int Get_Map_Height()  {return map_height;}	// Returns the height of the map in tiles		int Add_Layer(CWindraw *windraw); // Adds one layer to the map		void Add_Tileset(int tile_index,  // Adds one tileset to the map						 char *filename);  		int Draw_Tiles(HDC dest_hdc);	  // Draw all the tiles		int Load(char *filename);		  // Loads a map from disk				  		void Reset();					  // Resets all the variables to clear the way for the new data		int Save(char *filename);		  // Save the current map				~CMap(); // Frees up the memory	protected:		// protected members		char mapname[80],   // Name of the map			 tileset[80];   // Name of the tileset to use		int start_tile,  // Index of the starting tile			x,y,		 // Absolute coords of the map for scrolling purposes			map_width,   // Width of the map in tiles			map_height,  // Height of the map in tiles			scrn_width,  // Width of the screen in tiles			scrn_height, // Height of the screen in tiles			tile_width,  // Width of each tile			tile_height; // Height of each tile		float scale; // scale to use for zooming		vector<CLayer*> layers; // Vector of layers		HDC hdc;		 // HDC to use for tilesets		HBITMAP bmp;	 // bmp to use for tilesets		HBITMAP old_bmp; // used to save old bmp};
Sidenote
Storing HDC like you do is a no-no. HDC are global resources, and you should release them when you don't use them anymore. Moreover, it seems you are trying to duplicate them - because you are storing them. Using this you may create a system wide HDC shortage which could result in undefined behaviors (unpaint buttons or control are the first symptomes).

Are you sure you really need it? And, if you need it, are you sure you really want to clone it?

Regards,
Well I use this class in a tilemap editor, and I do release the hdc in the deconstructor. This isn't the only class that does this though. I also have CLayer and CTile, each of which have their own HBITMAPs and HDCs. A CMap is an array of CLayers, which in turn are arrays of CTiles. But like I said, all three classes release their resources when they go out of scope. I couldn't find any other way to have an array of bitmaps that I could blit to a backbuffer. Before I started the project I suspected that this wasn't exactly the best way, but the response from a few people at GT.com gave me the impression that it was a good way to go. This was a looooong time ago, so don't ask me for the thread or anything cause I won't remember lol. But is there a better way to do this?
Any suggestions?

This topic is closed to new replies.

Advertisement