From 8f719a7b2d8de79d8f456ef30ddde5f30100f18a Mon Sep 17 00:00:00 2001 From: rubidium Date: Sun, 5 Aug 2007 21:20:55 +0000 Subject: [PATCH] (svn r10799) -Fix: only calling QuickFree and not the destructor on pool cleanups might cause memory leaks due to the way C++ works. --- src/depot.cpp | 2 ++ src/group.h | 2 -- src/group_cmd.cpp | 7 +------ src/oldpool.cpp | 2 ++ src/oldpool.h | 39 +++++++++++++++++++++++---------------- src/signs.cpp | 7 +------ src/signs.h | 2 -- src/station.cpp | 13 +++++-------- src/station.h | 2 -- src/town.h | 2 -- src/town_cmd.cpp | 10 ++++------ src/vehicle.cpp | 12 ++++++------ src/vehicle.h | 2 -- src/waypoint.cpp | 11 ++++------- src/waypoint.h | 2 -- 15 files changed, 48 insertions(+), 67 deletions(-) diff --git a/src/depot.cpp b/src/depot.cpp index 63c6629e1f..2f6852bcc9 100644 --- a/src/depot.cpp +++ b/src/depot.cpp @@ -37,6 +37,8 @@ Depot *GetDepotByTile(TileIndex tile) */ Depot::~Depot() { + if (CleaningPool()) return; + /* Clear the depot from all order-lists */ RemoveOrderFromAllVehicles(OT_GOTO_DEPOT, this->index); diff --git a/src/group.h b/src/group.h index b4303e70c6..e5c10a59f2 100644 --- a/src/group.h +++ b/src/group.h @@ -29,8 +29,6 @@ struct Group : PoolItem { Group(StringID str = STR_NULL); virtual ~Group(); - void QuickFree(); - bool IsValid() const; }; diff --git a/src/group_cmd.cpp b/src/group_cmd.cpp index b8e68fbbf1..3ac42e2fdf 100644 --- a/src/group_cmd.cpp +++ b/src/group_cmd.cpp @@ -49,14 +49,9 @@ Group::Group(StringID str) } Group::~Group() -{ - this->QuickFree(); - this->string_id = STR_NULL; -} - -void Group::QuickFree() { DeleteName(this->string_id); + this->string_id = STR_NULL; } bool Group::IsValid() const diff --git a/src/oldpool.cpp b/src/oldpool.cpp index 3cc5c83911..df4731777d 100644 --- a/src/oldpool.cpp +++ b/src/oldpool.cpp @@ -18,6 +18,7 @@ void OldMemoryPoolBase::CleanPool() 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 @@ void OldMemoryPoolBase::CleanPool() } free(this->blocks[i]); } + this->cleaning_pool = false; /* Free the block itself */ free(this->blocks); diff --git a/src/oldpool.h b/src/oldpool.h index 38a3734742..caeabccaba 100644 --- a/src/oldpool.h +++ b/src/oldpool.h @@ -25,7 +25,7 @@ protected: 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 @@ protected: 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 @@ public: { return this->name; } + + /** + * Is the pool in the cleaning phase? + * @return true if it is + */ + inline bool CleaningPool() const + { + return this->cleaning_pool; + } }; template @@ -121,7 +131,6 @@ static void PoolNewBlock(uint start_item) /** * 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 @@ static void PoolCleanBlock(uint start_item, uint end_item) { for (uint i = start_item; i <= end_item; i++) { T *t = Tpool->Get(i); - if (t->IsValid()) { - t->QuickFree(); - } + delete t; } } @@ -156,15 +163,6 @@ struct PoolItem { if (this->index < Tpool->first_free_index) Tpool->first_free_index = this->index; } - /** - * 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) @@ -241,7 +239,7 @@ protected: * 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 @@ protected: * @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 @@ protected: return NULL; } + + /** + * Are we cleaning this pool? + * @return true if we are + */ + static inline bool CleaningPool() + { + return Tpool->CleaningPool(); + } }; diff --git a/src/signs.cpp b/src/signs.cpp index 8e05f2c018..f8ad95f45c 100644 --- a/src/signs.cpp +++ b/src/signs.cpp @@ -27,14 +27,9 @@ Sign::Sign(StringID string) } Sign::~Sign() -{ - this->QuickFree(); - this->str = STR_NULL; -} - -void Sign::QuickFree() { DeleteName(this->str); + this->str = STR_NULL; } /** diff --git a/src/signs.h b/src/signs.h index d335788075..05af404eb3 100644 --- a/src/signs.h +++ b/src/signs.h @@ -27,8 +27,6 @@ struct Sign : PoolItem { ~Sign(); bool IsValid() const { return this->str != STR_NULL; } - - void QuickFree(); }; enum { diff --git a/src/station.cpp b/src/station.cpp index 4633f1d188..150719e8fd 100644 --- a/src/station.cpp +++ b/src/station.cpp @@ -64,6 +64,11 @@ Station::~Station() { 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 @@ Station::~Station() 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 diff --git a/src/station.h b/src/station.h index b6e3596be2..866375a42a 100644 --- a/src/station.h +++ b/src/station.h @@ -159,8 +159,6 @@ struct Station : PoolItem { 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; diff --git a/src/town.h b/src/town.h index bda85ff78e..6d57071464 100644 --- a/src/town.h +++ b/src/town.h @@ -160,8 +160,6 @@ struct Town : PoolItem { ~Town(); bool IsValid() const { return this->xy != 0; } - - void QuickFree(); }; struct HouseSpec { diff --git a/src/town_cmd.cpp b/src/town_cmd.cpp index 4fa9a8c857..48b20ffc75 100644 --- a/src/town_cmd.cpp +++ b/src/town_cmd.cpp @@ -53,6 +53,10 @@ Town::Town(TileIndex tile) Town::~Town() { + DeleteName(this->townnametype); + + if (CleaningPool()) return; + Industry *i; /* Delete town authority window @@ -87,15 +91,9 @@ Town::~Town() MarkWholeScreenDirty(); - this->QuickFree(); this->xy = 0; } -void Town::QuickFree() -{ - DeleteName(this->townnametype); -} - // Local static int _grow_town_result; diff --git a/src/vehicle.cpp b/src/vehicle.cpp index c845a00b0a..78b857c354 100644 --- a/src/vehicle.cpp +++ b/src/vehicle.cpp @@ -564,6 +564,8 @@ bool IsEngineCountable(const Vehicle *v) 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 @@ void Vehicle::PreDestructor() 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 @@ void Vehicle::PreDestructor() 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 @@ Vehicle::~Vehicle() new (this) InvalidVehicle(); } -void Vehicle::QuickFree() -{ - DeleteName(this->string_id); -} - /** * Deletes all vehicles in a chain. * @param v The first vehicle in the chain. diff --git a/src/vehicle.h b/src/vehicle.h index 7439877636..63f4203ee8 100644 --- a/src/vehicle.h +++ b/src/vehicle.h @@ -352,8 +352,6 @@ struct Vehicle : PoolItem { /** We want to 'destruct' the right class. */ virtual ~Vehicle(); - void QuickFree(); - void BeginLoading(); void LeaveStation(); diff --git a/src/waypoint.cpp b/src/waypoint.cpp index 425c7a503b..9847c98700 100644 --- a/src/waypoint.cpp +++ b/src/waypoint.cpp @@ -404,17 +404,14 @@ Waypoint::Waypoint(TileIndex tile) 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 diff --git a/src/waypoint.h b/src/waypoint.h index af24b19068..4926d13134 100644 --- a/src/waypoint.h +++ b/src/waypoint.h @@ -30,8 +30,6 @@ struct Waypoint : PoolItem { Waypoint(TileIndex tile = 0); ~Waypoint(); - void QuickFree(); - bool IsValid() const; };