widelands-dev team mailing list archive

[Kiscsirke <csirkeee@xxxxxxxxx>]

Kiscsirke has proposed merging lp:~csirkeee/widelands/memory-leaks-2 into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~csirkeee/widelands/memory-leaks-2/+merge/150290

I've tried looking into memory leaks in connection with bug #1096453. This fixes some that I've found with Valgrind on Linux, and with DrMemory on windows. It doesn't significantly decrease memory consumption (I'm still seeing ~50MB additional memory used every time I launch a new game for example), but it still makes the code more correct.

Also included is a small change to CMakeLists that adds the Boost include directory even in Release mode (that I'm surprised everybody else doesn't need).
-- 
https://code.launchpad.net/~csirkeee/widelands/memory-leaks-2/+merge/150290
Your team Widelands Developers is requested to review the proposed merge of lp:~csirkeee/widelands/memory-leaks-2 into lp:widelands.

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2013-01-27 14:13:03 +0000
+++ CMakeLists.txt	2013-02-25 11:03:58 +0000
@@ -161,39 +161,44 @@
 set (PARAMETER_COMPILERFLAG_EXTRAWARNINGS "-Wshadow -Wsign-promo -Wundef -Wunused -Wformat -Wformat-nonliteral -Wformat-security")
 set (PARAMETER_COMPILERFLAG_GCCWARNINGS "-Wlogical-op -Wsync-nand -Wtrampolines")
 set (PARAMETER_COMPILERFLAG_STRICT "-Werror -Wno-error=old-style-cast -Wno-error=deprecated-declarations -fdiagnostics-show-option")
-IF (CMAKE_BUILD_TYPE STREQUAL "Debug")
-  include(CheckCXXCompilerFlag) #this include should be safe
-  CHECK_CXX_COMPILER_FLAG(${PARAMETER_COMPILERFLAG_OLDSTYLECAST_EXTENDED} Compiler_old-style-cast_supported)
-  IF (Compiler_old-style-cast_supported)
-    EXEC_PROGRAM(${CMAKE_CXX_COMPILER}
-      ARGS --version
-      OUTPUT_VARIABLE WLBUILD_COMPILERVERSION
-    )
-    STRING(REGEX REPLACE ".*([0-9])\\.([0-9])\\.([0-9]).*" "\\1.\\2.\\3" WLBUILD_COMPILERVERSION_REP ${WLBUILD_COMPILERVERSION})
-    IF (WLBUILD_COMPILERVERSION_REP STREQUAL "4.3.2" OR WLBUILD_COMPILERVERSION_REP STREQUAL "4.3.1")
-      message("===== ATTENTION ===================================================")
-      message("Your compiler has been identified as GCC 4.3.1 or GCC 4.3.2")
-      message(" ")
-      message("There is a bug regarding this version, see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38000";)
-      message("and https://bugs.launchpad.net/widelands/+bug/549479 for details.")
-      message("Build will disable -isystem usage, expect lots of warnings from Boost header files.")
-      message("Widelands should still compile and link correctly.")
-      message("If the compiler identification is correct, please consider updating your gcc.")
-      message("This warning message will disappear at GCC 4.3.3 and higher.")
-      message("If you feel this is wrong, please submit a bug with widelands (not with GCC!)")
-      message("and add the following information how cmake sees your C++ compiler:")
-      message(" ")
-      message("--------------------------------")
-      message("${WLBUILD_COMPILERVERSION}")
-      message("--------------------------------")
-      message(" ")
-      message("Thank you for taking your time to read this.")
-      message("===================================================================")
-      set (WL_COMPILERFLAG_OLDSTYLECAST " ${PARAMETER_COMPILERFLAG_OLDSTYLECAST}") #the space is on purpose!
-    ELSE (WLBUILD_COMPILERVERSION_REP STREQUAL "4.3.2" OR WLBUILD_COMPILERVERSION_REP STREQUAL "4.3.1")
+include(CheckCXXCompilerFlag) #this include should be safe
+CHECK_CXX_COMPILER_FLAG(${PARAMETER_COMPILERFLAG_OLDSTYLECAST_EXTENDED} Compiler_old-style-cast_supported)
+IF (Compiler_old-style-cast_supported)
+  EXEC_PROGRAM(${CMAKE_CXX_COMPILER}
+    ARGS --version
+    OUTPUT_VARIABLE WLBUILD_COMPILERVERSION
+  )
+  STRING(REGEX REPLACE ".*([0-9])\\.([0-9])\\.([0-9]).*" "\\1.\\2.\\3" WLBUILD_COMPILERVERSION_REP ${WLBUILD_COMPILERVERSION})
+  IF (WLBUILD_COMPILERVERSION_REP STREQUAL "4.3.2" OR WLBUILD_COMPILERVERSION_REP STREQUAL "4.3.1")
+    message("===== ATTENTION ===================================================")
+    message("Your compiler has been identified as GCC 4.3.1 or GCC 4.3.2")
+    message(" ")
+    message("There is a bug regarding this version, see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=38000";)
+    message("and https://bugs.launchpad.net/widelands/+bug/549479 for details.")
+    message("Build will disable -isystem usage, expect lots of warnings from Boost header files.")
+    message("Widelands should still compile and link correctly.")
+    message("If the compiler identification is correct, please consider updating your gcc.")
+    message("This warning message will disappear at GCC 4.3.3 and higher.")
+    message("If you feel this is wrong, please submit a bug with widelands (not with GCC!)")
+    message("and add the following information how cmake sees your C++ compiler:")
+    message(" ")
+    message("--------------------------------")
+    message("${WLBUILD_COMPILERVERSION}")
+    message("--------------------------------")
+    message(" ")
+    message("Thank you for taking your time to read this.")
+    message("===================================================================")
+    set (WL_COMPILERFLAG_OLDSTYLECAST " ${PARAMETER_COMPILERFLAG_OLDSTYLECAST}") #the space is on purpose!
+  ELSE (WLBUILD_COMPILERVERSION_REP STREQUAL "4.3.2" OR WLBUILD_COMPILERVERSION_REP STREQUAL "4.3.1")
+    IF (CMAKE_BUILD_TYPE STREQUAL "Debug")
       set (WL_COMPILERFLAG_OLDSTYLECAST " ${PARAMETER_COMPILERFLAG_OLDSTYLECAST_EXTENDED}") #the space is on purpose!
-    ENDIF (WLBUILD_COMPILERVERSION_REP STREQUAL "4.3.2" OR WLBUILD_COMPILERVERSION_REP STREQUAL "4.3.1")
-  ENDIF (Compiler_old-style-cast_supported)
+    ELSE (CMAKE_BUILD_TYPE STREQUAL "Debug")
+      include_directories(SYSTEM path ${Boost_INCLUDE_DIR})
+    ENDIF (CMAKE_BUILD_TYPE STREQUAL "Debug")
+  ENDIF (WLBUILD_COMPILERVERSION_REP STREQUAL "4.3.2" OR WLBUILD_COMPILERVERSION_REP STREQUAL "4.3.1")
+ENDIF (Compiler_old-style-cast_supported)
+
+IF (CMAKE_BUILD_TYPE STREQUAL "Debug")
 
   CHECK_CXX_COMPILER_FLAG(${PARAMETER_COMPILERFLAG_GENERICWARNINGS} Compiler_generic_warnings_supported)
   IF (Compiler_generic_warnings_supported)

=== modified file 'src/graphic/graphic.cc'
--- src/graphic/graphic.cc	2013-02-10 16:08:37 +0000
+++ src/graphic/graphic.cc	2013-02-25 11:03:58 +0000
@@ -20,6 +20,8 @@
 #include <cstring>
 #include <iostream>
 
+#include <boost/foreach.hpp>
+
 #include <SDL_image.h>
 #include <config.h>
 
@@ -298,6 +300,8 @@
 */
 Graphic::~Graphic()
 {
+	BOOST_FOREACH(Texture* texture, m_maptextures)
+		delete texture;
 	delete m_rendertarget;
 
 #if USE_OPENGL

=== modified file 'src/logic/cmd_luacoroutine.cc'
--- src/logic/cmd_luacoroutine.cc	2013-02-10 19:36:24 +0000
+++ src/logic/cmd_luacoroutine.cc	2013-02-25 11:03:58 +0000
@@ -35,8 +35,10 @@
 		int rv = m_cr->resume(&sleeptime);
 		if (rv == LuaCoroutine::YIELDED) {
 			game.enqueue_command(new Widelands::Cmd_LuaCoroutine(sleeptime, m_cr));
+			m_cr = 0;  // Remove our ownership so we don't delete.
 		} else if (rv == LuaCoroutine::DONE) {
 			delete m_cr;
+			m_cr = 0;
 		}
 	} catch (LuaError & e) {
 		log("Error in Lua Coroutine\n");

=== modified file 'src/logic/cmd_luacoroutine.h'
--- src/logic/cmd_luacoroutine.h	2012-09-21 21:36:07 +0000
+++ src/logic/cmd_luacoroutine.h	2013-02-25 11:03:58 +0000
@@ -32,6 +32,10 @@
 	Cmd_LuaCoroutine(int32_t const _duetime, LuaCoroutine * const cr) :
 		GameLogicCommand(_duetime), m_cr(cr) {}
 
+	~Cmd_LuaCoroutine() {
+		delete m_cr;
+	}
+
 	// Write these commands to a file (for savegames)
 	void Write(FileWrite &, Editor_Game_Base &, Map_Map_Object_Saver  &);
 	void Read (FileRead  &, Editor_Game_Base &, Map_Map_Object_Loader &);

=== modified file 'src/logic/tribe.cc'
--- src/logic/tribe.cc	2013-02-10 19:36:24 +0000
+++ src/logic/tribe.cc	2013-02-25 11:03:58 +0000
@@ -48,6 +48,7 @@
 
 #include <iostream>
 #include <boost/algorithm/string.hpp>
+#include <boost/scoped_ptr.hpp>
 
 using namespace std;
 
@@ -98,7 +99,7 @@
 			if (Section * const section = root_conf.get_section("compatibility_wares")) {
 				while (Section::Value const * const v = section->get_next_val()) {
 					log("Compatibility ware \"%s\"=\"%s\" loaded.\n", v->get_name(), v->get_string());
-					m_compatibility_wares[* new std::string(v->get_name())] = * new std::string(v->get_string());
+					m_compatibility_wares[std::string(v->get_name())] = std::string(v->get_string());
 				}
 			}
 
@@ -293,9 +294,11 @@
 			}
 
 			// Register Lua scripts
-			if (g_fs->IsDirectory(path + "scripting"))
+			if (g_fs->IsDirectory(path + "scripting")) {
+				boost::scoped_ptr<FileSystem> sub_fs(&g_fs->MakeSubFileSystem(path));
 				egbase.lua().register_scripts
-					(g_fs->MakeSubFileSystem(path), "tribe_" + tribename);
+					(*sub_fs.get(), "tribe_" + tribename);
+			}
 
 			// Read initializations -- all scripts are initializations currently
 			ScriptContainer & scripts =

=== modified file 'src/network/netclient.cc'
--- src/network/netclient.cc	2013-02-10 20:07:27 +0000
+++ src/network/netclient.cc	2013-02-25 11:03:58 +0000
@@ -45,6 +45,7 @@
 #include "ui_basic/progresswindow.h"
 
 #include <boost/lexical_cast.hpp>
+#include <boost/scoped_ptr.hpp>
 #include <config.h>
 #ifndef HAVE_VARARRAY
 #include <climits>
@@ -904,8 +905,9 @@
 			LuaInterface * lua = create_LuaInterface();
 			std::string path = "tribes/" + info.name;
 			if (g_fs->IsDirectory(path)) {
+				boost::scoped_ptr<FileSystem> sub_fs(&g_fs->MakeSubFileSystem(path));
 				lua->register_scripts
-					(g_fs->MakeSubFileSystem(path), "tribe_" + info.name);
+					(*sub_fs.get(), "tribe_" + info.name);
 			}
 
 			for (uint8_t j = packet.Unsigned8(); j; --j) {

=== modified file 'src/sound/songset.cc'
--- src/sound/songset.cc	2013-02-10 19:36:24 +0000
+++ src/sound/songset.cc	2013-02-25 11:03:58 +0000
@@ -35,6 +35,11 @@
 
 	if (m_m)
 		Mix_FreeMusic(m_m);
+
+	if (m_rwops) {
+		SDL_FreeRW(m_rwops);
+		m_fr.Close();
+	}
 }
 
 /** Append a song to the end of the songset
@@ -73,16 +78,27 @@
 	//first, close the previous song and remove it from memory
 	if (m_m) {
 		Mix_FreeMusic(m_m);
+		m_m = 0;
+	}
+
+	if (m_rwops) {
+		SDL_FreeRW(m_rwops);
+		m_rwops = 0;
 		m_fr.Close();
 	}
 
 	//then open the new song
-	if (m_fr.TryOpen(*g_fs, filename.c_str()))
-		m_rwops = SDL_RWFromMem(m_fr.Data(0), m_fr.GetSize());
+	if (m_fr.TryOpen(*g_fs, filename.c_str())) {
+		if (!(m_rwops = SDL_RWFromMem(m_fr.Data(0), m_fr.GetSize()))) {
+			m_fr.Close();  // m_fr should be Open iff m_rwops != 0
+			return 0;
+		}
+	}
 	else
 		return 0;
 
-	m_m = Mix_LoadMUS_RW(m_rwops);
+	if (m_rwops)
+		m_m = Mix_LoadMUS_RW(m_rwops);
 
 	if (m_m)
 		log("Sound_Handler: loaded song \"%s\"\n", filename.c_str());

=== modified file 'src/sound/sound_handler.cc'
--- src/sound/sound_handler.cc	2013-02-10 19:36:24 +0000
+++ src/sound/sound_handler.cc	2013-02-25 11:03:58 +0000
@@ -117,9 +117,7 @@
 	if
 		(SDL_InitSubSystem(SDL_INIT_AUDIO) == -1
 		 or
-		 Mix_OpenAudio(MIX_DEFAULT_FREQUENCY, MIX_DEFAULT_FORMAT, 2, bufsize)
-		 ==
-		 -1)
+		 Mix_OpenAudio(MIX_DEFAULT_FREQUENCY, MIX_DEFAULT_FORMAT, 2, bufsize) == -1)
 	{
 		SDL_QuitSubSystem(SDL_INIT_AUDIO);
 		log("WARNING: Failed to initialize sound system: %s\n", Mix_GetError());
@@ -159,7 +157,7 @@
 	if (SDL_InitSubSystem(SDL_INIT_AUDIO) == -1) {
 		log ("audio error %s\n", SDL_GetError());
 	}
-	char * text = new char[15];
+	char * text = new char[21];
 	SDL_AudioDriverName(text, 20);
 	log("SDL_AUDIODRIVER %s\n", text);
 	delete text;

=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2013-02-10 20:07:27 +0000
+++ src/wlapplication.cc	2013-02-25 11:03:58 +0000
@@ -349,6 +349,8 @@
 		std::cerr.flush();
 		fclose(stderr);
 	}
+
+	SDL_Quit();
 }
 
 /**