changeset 7413:571c87eed06c draft

(svn r10799) -Fix: only calling QuickFree and not the destructor on pool cleanups might cause memory leaks due to the way C++ works.
author rubidium <rubidium@openttd.org>
date Sun, 05 Aug 2007 21:20:55 +0000
parents 6682e85ffa07
children 2496ac7d0086
files src/depot.cpp src/group.h src/group_cmd.cpp src/oldpool.cpp src/oldpool.h src/signs.cpp src/signs.h src/station.cpp src/station.h src/town.h src/town_cmd.cpp src/vehicle.cpp src/vehicle.h src/waypoint.cpp src/waypoint.h
diffstat 15 files changed, 48 insertions(+), 67 deletions(-) [+]
line wrap: on
line diff
--- a/src/depot.cpp
+++ b/src/depot.cpp
@@ -37,6 +37,8 @@
  */
 Depot::~Depot()
 {
+	if (CleaningPool()) return;
+
 	/* Clear the depot from all order-lists */
 	RemoveOrderFromAllVehicles(OT_GOTO_DEPOT, this->index);
 
--- a/src/group.h
+++ b/src/group.h
@@ -29,8 +29,6 @@
 	Group(StringID str = STR_NULL);
 	virtual ~Group();
 
-	void QuickFree();
-
 	bool IsValid() const;
 };
 
--- a/src/group_cmd.cpp
+++ b/src/group_cmd.cpp
@@ -50,15 +50,10 @@
 
 Group::~Group()
 {
-	this->QuickFree();
+	DeleteName(this->string_id);
 	this->string_id = STR_NULL;
 }
 
-void Group::QuickFree()
-{
-	DeleteName(this->string_id);
-}
-
 bool Group::IsValid() const
 {
 	return this->string_id != STR_NULL;
--- a/src/oldpool.cpp
+++ b/src/oldpool.cpp
@@ -18,6 +18,7 @@
 
 	DEBUG(misc, 4, "[Pool] (%s) cleaning pool..", this->name);
 
+	this->cleaning_pool = true;
 	/* Free all blocks */
 	for (i = 0; i < this->current_blocks; i++) {
 		if (this->clean_block_proc != NULL) {
@@ -25,6 +26,7 @@
 		}
 		free(this->blocks[i]);
 	}
+	this->cleaning_pool = false;
 
 	/* Free the block itself */
 	free(this->blocks);
--- a/src/oldpool.h
+++ b/src/oldpool.h
@@ -25,7 +25,7 @@
 				OldMemoryPoolNewBlock *new_block_proc, OldMemoryPoolCleanBlock *clean_block_proc) :
 		name(name), max_blocks(max_blocks), block_size_bits(block_size_bits),
 		new_block_proc(new_block_proc), clean_block_proc(clean_block_proc), current_blocks(0),
-		total_items(0), item_size(item_size), first_free_index(0), blocks(NULL) {}
+		total_items(0), cleaning_pool(false), item_size(item_size), first_free_index(0), blocks(NULL) {}
 
 	const char* name;     ///< Name of the pool (just for debugging)
 
@@ -40,6 +40,7 @@
 	uint current_blocks;        ///< How many blocks we have in our pool
 	uint total_items;           ///< How many items we now have in this pool
 
+	bool cleaning_pool;         ///< Are we currently cleaning the pool?
 public:
 	const uint item_size;       ///< How many bytes one block is
 	uint first_free_index;      ///< The index of the first free pool item in this pool
@@ -84,6 +85,15 @@
 	{
 		return this->name;
 	}
+
+	/**
+	 * Is the pool in the cleaning phase?
+	 * @return true if it is
+	 */
+	inline bool CleaningPool() const
+	{
+		return this->cleaning_pool;
+	}
 };
 
 template <typename T>
@@ -121,7 +131,6 @@
 
 /**
  * Generic function to free a new block in a pool.
- * This function uses QuickFree that is intended to only free memory that would be lost if the pool is freed.
  * @param start_item the first item that needs to be cleaned
  * @param end_item   the last item that needs to be cleaned
  */
@@ -130,9 +139,7 @@
 {
 	for (uint i = start_item; i <= end_item; i++) {
 		T *t = Tpool->Get(i);
-		if (t->IsValid()) {
-			t->QuickFree();
-		}
+		delete t;
 	}
 }
 
@@ -157,15 +164,6 @@
 	}
 
 	/**
-	 * Called on each object when the pool is being destroyed, so one
-	 * can free allocated memory without the need for freeing for
-	 * example orders.
-	 */
-	virtual void QuickFree()
-	{
-	}
-
-	/**
 	 * An overriden version of new that allocates memory on the pool.
 	 * @param size the size of the variable (unused)
 	 * @return the memory that is 'allocated'
@@ -241,7 +239,7 @@
 	 * Allocate a pool item; possibly allocate a new block in the pool.
 	 * @return the allocated pool item (or NULL when the pool is full).
 	 */
-	static T *AllocateRaw()
+	static inline T *AllocateRaw()
 	{
 		return AllocateRaw(Tpool->first_free_index);
 	}
@@ -251,7 +249,7 @@
 	 * @param first the first pool item to start searching
 	 * @return the allocated pool item (or NULL when the pool is full).
 	 */
-	static T *AllocateRaw(uint &first)
+	static inline T *AllocateRaw(uint &first)
 	{
 		uint last_minus_one = Tpool->GetSize() - 1;
 
@@ -271,6 +269,15 @@
 
 		return NULL;
 	}
+
+	/**
+	 * Are we cleaning this pool?
+	 * @return true if we are
+	 */
+	static inline bool CleaningPool()
+	{
+		return Tpool->CleaningPool();
+	}
 };
 
 
--- a/src/signs.cpp
+++ b/src/signs.cpp
@@ -28,15 +28,10 @@
 
 Sign::~Sign()
 {
-	this->QuickFree();
+	DeleteName(this->str);
 	this->str = STR_NULL;
 }
 
-void Sign::QuickFree()
-{
-	DeleteName(this->str);
-}
-
 /**
  *
  * Update the coordinate of one sign
--- a/src/signs.h
+++ b/src/signs.h
@@ -27,8 +27,6 @@
 	~Sign();
 
 	bool IsValid() const { return this->str != STR_NULL; }
-
-	void QuickFree();
 };
 
 enum {
--- a/src/station.cpp
+++ b/src/station.cpp
@@ -64,6 +64,11 @@
 {
 	DEBUG(station, cDebugCtorLevel, "I-%3d", index);
 
+	DeleteName(this->string_id);
+	free(this->speclist);
+
+	if (CleaningPool()) return;
+
 	MarkDirty();
 	RebuildStationLists();
 	InvalidateWindowClasses(WC_STATION_LIST);
@@ -81,14 +86,6 @@
 	for (CargoID c = 0; c < NUM_CARGO; c++) {
 		goods[c].cargo.Truncate(0);
 	}
-
-	this->QuickFree();
-}
-
-void Station::QuickFree()
-{
-	DeleteName(this->string_id);
-	free(this->speclist);
 }
 
 /** Called when new facility is built on the station. If it is the first facility
--- a/src/station.h
+++ b/src/station.h
@@ -159,8 +159,6 @@
 	Station(TileIndex tile = 0);
 	virtual ~Station();
 
-	void QuickFree();
-
 	void AddFacility(byte new_facility_bit, TileIndex facil_xy);
 	void MarkDirty() const;
 	void MarkTilesDirty(bool cargo_change) const;
--- a/src/town.h
+++ b/src/town.h
@@ -160,8 +160,6 @@
 	~Town();
 
 	bool IsValid() const { return this->xy != 0; }
-
-	void QuickFree();
 };
 
 struct HouseSpec {
--- a/src/town_cmd.cpp
+++ b/src/town_cmd.cpp
@@ -53,6 +53,10 @@
 
 Town::~Town()
 {
+	DeleteName(this->townnametype);
+
+	if (CleaningPool()) return;
+
 	Industry *i;
 
 	/* Delete town authority window
@@ -87,15 +91,9 @@
 
 	MarkWholeScreenDirty();
 
-	this->QuickFree();
 	this->xy = 0;
 }
 
-void Town::QuickFree()
-{
-	DeleteName(this->townnametype);
-}
-
 // Local
 static int _grow_town_result;
 
--- a/src/vehicle.cpp
+++ b/src/vehicle.cpp
@@ -564,6 +564,8 @@
 
 void Vehicle::PreDestructor()
 {
+	if (CleaningPool()) return;
+
 	if (IsValidStationID(this->last_station_visited)) {
 		GetStation(this->last_station_visited)->loading_vehicles.remove(this);
 
@@ -579,7 +581,6 @@
 		if (this->IsPrimaryVehicle()) DecreaseGroupNumVehicle(this->group_id);
 	}
 
-	this->QuickFree();
 	if (this->type == VEH_ROAD) ClearSlot(this);
 
 	if (this->type != VEH_TRAIN || (this->type == VEH_TRAIN && (IsFrontEngine(this) || IsFreeWagon(this)))) {
@@ -599,6 +600,10 @@
 
 Vehicle::~Vehicle()
 {
+	DeleteName(this->string_id);
+
+	if (CleaningPool()) return;
+
 	UpdateVehiclePosHash(this, INVALID_COORD, 0);
 	this->next_hash = NULL;
 	this->next_new_hash = NULL;
@@ -608,11 +613,6 @@
 	new (this) InvalidVehicle();
 }
 
-void Vehicle::QuickFree()
-{
-	DeleteName(this->string_id);
-}
-
 /**
  * Deletes all vehicles in a chain.
  * @param v The first vehicle in the chain.
--- a/src/vehicle.h
+++ b/src/vehicle.h
@@ -352,8 +352,6 @@
 	/** We want to 'destruct' the right class. */
 	virtual ~Vehicle();
 
-	void QuickFree();
-
 	void BeginLoading();
 	void LeaveStation();
 
--- a/src/waypoint.cpp
+++ b/src/waypoint.cpp
@@ -404,17 +404,14 @@
 
 Waypoint::~Waypoint()
 {
+	if (this->string != STR_NULL) DeleteName(this->string);
+
+	if (CleaningPool()) return;
+
 	RemoveOrderFromAllVehicles(OT_GOTO_WAYPOINT, this->index);
 
 	RedrawWaypointSign(this);
 	this->xy = 0;
-
-	this->QuickFree();
-}
-
-void Waypoint::QuickFree()
-{
-	if (this->string != STR_NULL) DeleteName(this->string);
 }
 
 bool Waypoint::IsValid() const
--- a/src/waypoint.h
+++ b/src/waypoint.h
@@ -30,8 +30,6 @@
 	Waypoint(TileIndex tile = 0);
 	~Waypoint();
 
-	void QuickFree();
-
 	bool IsValid() const;
 };