From 149445c79e61da65c9fa652dca7fe361a028686a Mon Sep 17 00:00:00 2001 From: Peter Nelson Date: Wed, 26 Mar 2025 11:58:56 +0000 Subject: [PATCH 1/3] Codechange: Keep Squirrel engine in unique_ptr. --- src/ai/ai_scanner.cpp | 2 +- src/script/script_scanner.cpp | 9 ++------- src/script/script_scanner.hpp | 4 ++-- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/ai/ai_scanner.cpp b/src/ai/ai_scanner.cpp index 281b8925d3..1c7b089470 100644 --- a/src/ai/ai_scanner.cpp +++ b/src/ai/ai_scanner.cpp @@ -32,7 +32,7 @@ void AIScannerInfo::Initialize() { ScriptScanner::Initialize("AIScanner"); - ScriptAllocatorScope alloc_scope(this->engine); + ScriptAllocatorScope alloc_scope(this->engine.get()); /* Create the dummy AI */ this->main_script = "%_dummy"; diff --git a/src/script/script_scanner.cpp b/src/script/script_scanner.cpp index 0aea257240..102ce454d4 100644 --- a/src/script/script_scanner.cpp +++ b/src/script/script_scanner.cpp @@ -44,10 +44,7 @@ bool ScriptScanner::AddFile(const std::string &filename, size_t, const std::stri return true; } -ScriptScanner::ScriptScanner() : - engine(nullptr) -{ -} +ScriptScanner::ScriptScanner() = default; void ScriptScanner::ResetEngine() { @@ -58,7 +55,7 @@ void ScriptScanner::ResetEngine() void ScriptScanner::Initialize(std::string_view name) { - this->engine = new Squirrel(name); + this->engine = std::make_unique(name); this->RescanDir(); @@ -68,8 +65,6 @@ void ScriptScanner::Initialize(std::string_view name) ScriptScanner::~ScriptScanner() { this->Reset(); - - delete this->engine; } void ScriptScanner::RescanDir() diff --git a/src/script/script_scanner.hpp b/src/script/script_scanner.hpp index e05da0b2ba..e70ffe7b0b 100644 --- a/src/script/script_scanner.hpp +++ b/src/script/script_scanner.hpp @@ -26,7 +26,7 @@ public: /** * Get the engine of the main squirrel handler (it indexes all available scripts). */ - class Squirrel *GetEngine() { return this->engine; } + class Squirrel *GetEngine() { return this->engine.get(); } /** * Get the current main script the ScanDir is currently tracking. @@ -84,7 +84,7 @@ public: void RescanDir(); protected: - class Squirrel *engine; ///< The engine we're scanning with. + std::unique_ptr engine; ///< The engine we're scanning with. std::string main_script; ///< The full path of the script. std::string tar_file; ///< If, which tar file the script was in. From f182d7ab6576321680a206048a3991080388b8d4 Mon Sep 17 00:00:00 2001 From: Peter Nelson Date: Mon, 28 Apr 2025 22:27:34 +0100 Subject: [PATCH 2/3] Codechange: Pass ScriptInfo by reference to IsSameScript. --- src/script/script_scanner.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/script/script_scanner.cpp b/src/script/script_scanner.cpp index 102ce454d4..506b6df51c 100644 --- a/src/script/script_scanner.cpp +++ b/src/script/script_scanner.cpp @@ -190,17 +190,17 @@ struct ScriptFileChecksumCreator : FileScanner { * @param info The script to get the shortname and md5 sum from. * @return True iff they're the same. */ -static bool IsSameScript(const ContentInfo &ci, bool md5sum, ScriptInfo *info, Subdirectory dir) +static bool IsSameScript(const ContentInfo &ci, bool md5sum, const ScriptInfo &info, Subdirectory dir) { uint32_t id = 0; - auto str = std::string_view{info->GetShortName()}.substr(0, 4); + auto str = std::string_view{info.GetShortName()}.substr(0, 4); for (size_t j = 0; j < str.size(); j++) id |= static_cast(str[j]) << (8 * j); if (id != ci.unique_id) return false; if (!md5sum) return true; ScriptFileChecksumCreator checksum(dir); - const auto &tar_filename = info->GetTarFile(); + const auto &tar_filename = info.GetTarFile(); TarList::iterator iter; if (!tar_filename.empty() && (iter = _tar_list[dir].find(tar_filename)) != _tar_list[dir].end()) { /* The main script is in a tar file, so find all files that @@ -219,7 +219,7 @@ static bool IsSameScript(const ContentInfo &ci, bool md5sum, ScriptInfo *info, S /* There'll always be at least 1 path separator character in a script * main script name as the search algorithm requires the main script to * be in a subdirectory of the script directory; so //main.nut. */ - const std::string &main_script = info->GetMainScript(); + const std::string &main_script = info.GetMainScript(); std::string path = main_script.substr(0, main_script.find_last_of(PATHSEPCHAR)); checksum.Scan(".nut", path); } @@ -230,7 +230,7 @@ static bool IsSameScript(const ContentInfo &ci, bool md5sum, ScriptInfo *info, S bool ScriptScanner::HasScript(const ContentInfo &ci, bool md5sum) { for (const auto &item : this->info_list) { - if (IsSameScript(ci, md5sum, item.second, this->GetDirectory())) return true; + if (IsSameScript(ci, md5sum, *item.second, this->GetDirectory())) return true; } return false; } @@ -238,7 +238,7 @@ bool ScriptScanner::HasScript(const ContentInfo &ci, bool md5sum) std::optional ScriptScanner::FindMainScript(const ContentInfo &ci, bool md5sum) { for (const auto &item : this->info_list) { - if (IsSameScript(ci, md5sum, item.second, this->GetDirectory())) return item.second->GetMainScript(); + if (IsSameScript(ci, md5sum, *item.second, this->GetDirectory())) return item.second->GetMainScript(); } return std::nullopt; } From 7098320126b148bf600e05774931d7da87d15e9f Mon Sep 17 00:00:00 2001 From: Peter Nelson Date: Mon, 28 Apr 2025 22:26:47 +0100 Subject: [PATCH 3/3] Codechange: Use unique_ptr for ScriptInfo instances. Replaces raw pointers, slightly. --- src/ai/ai_info.cpp | 9 ++++----- src/game/game_info.cpp | 9 ++++----- src/game/game_scanner.cpp | 4 ++-- src/script/script_scanner.cpp | 21 ++++++++------------- src/script/script_scanner.hpp | 8 +++++--- 5 files changed, 23 insertions(+), 28 deletions(-) diff --git a/src/ai/ai_info.cpp b/src/ai/ai_info.cpp index 67fad8d494..9a2bfa0a85 100644 --- a/src/ai/ai_info.cpp +++ b/src/ai/ai_info.cpp @@ -90,7 +90,7 @@ template <> SQInteger PushClassName(HSQUIRRELVM vm) { sq /* Remove the link to the real instance, else it might get deleted by RegisterAI() */ sq_setinstanceup(vm, 2, nullptr); /* Register the AI to the base system */ - info->GetScanner()->RegisterScript(info); + info->GetScanner()->RegisterScript(std::unique_ptr{info}); return 0; } @@ -136,22 +136,21 @@ bool AIInfo::CanLoadFromVersion(int version) const /* static */ SQInteger AILibrary::Constructor(HSQUIRRELVM vm) { /* Create a new library */ - AILibrary *library = new AILibrary(); + auto library = std::make_unique(); SQInteger res = ScriptInfo::Constructor(vm, *library); if (res != 0) { - delete library; return res; } /* Cache the category */ if (!library->CheckMethod("GetCategory") || !library->engine->CallStringMethod(library->SQ_instance, "GetCategory", &library->category, MAX_GET_OPS)) { - delete library; return SQ_ERROR; } /* Register the Library to the base system */ - library->GetScanner()->RegisterScript(library); + ScriptScanner *scanner = library->GetScanner(); + scanner->RegisterScript(std::move(library)); return 0; } diff --git a/src/game/game_info.cpp b/src/game/game_info.cpp index edf5334604..e8b3ef6292 100644 --- a/src/game/game_info.cpp +++ b/src/game/game_info.cpp @@ -78,7 +78,7 @@ template <> SQInteger PushClassName(HSQUIRRELVM vm) { /* Remove the link to the real instance, else it might get deleted by RegisterGame() */ sq_setinstanceup(vm, 2, nullptr); /* Register the Game to the base system */ - info->GetScanner()->RegisterScript(info); + info->GetScanner()->RegisterScript(std::unique_ptr{info}); return 0; } @@ -106,22 +106,21 @@ bool GameInfo::CanLoadFromVersion(int version) const /* static */ SQInteger GameLibrary::Constructor(HSQUIRRELVM vm) { /* Create a new library */ - GameLibrary *library = new GameLibrary(); + auto library = std::make_unique(); SQInteger res = ScriptInfo::Constructor(vm, *library); if (res != 0) { - delete library; return res; } /* Cache the category */ if (!library->CheckMethod("GetCategory") || !library->engine->CallStringMethod(library->SQ_instance, "GetCategory", &library->category, MAX_GET_OPS)) { - delete library; return SQ_ERROR; } /* Register the Library to the base system */ - library->GetScanner()->RegisterScript(library); + ScriptScanner *scanner = library->GetScanner(); + scanner->RegisterScript(std::move(library)); return 0; } diff --git a/src/game/game_scanner.cpp b/src/game/game_scanner.cpp index e550772883..16f8c70bdc 100644 --- a/src/game/game_scanner.cpp +++ b/src/game/game_scanner.cpp @@ -92,8 +92,8 @@ GameLibrary *GameScannerLibrary::FindLibrary(const std::string &library, int ver std::string library_name = fmt::format("{}.{}", library, version); /* Check if the library + version exists */ - ScriptInfoList::iterator it = this->info_list.find(library_name); + auto it = this->info_list.find(library_name); if (it == this->info_list.end()) return nullptr; - return static_cast((*it).second); + return static_cast(it->second); } diff --git a/src/script/script_scanner.cpp b/src/script/script_scanner.cpp index 506b6df51c..ff7bece3b1 100644 --- a/src/script/script_scanner.cpp +++ b/src/script/script_scanner.cpp @@ -78,15 +78,12 @@ void ScriptScanner::RescanDir() void ScriptScanner::Reset() { - for (const auto &item : this->info_list) { - delete item.second; - } - this->info_list.clear(); this->info_single_list.clear(); + this->info_vector.clear(); } -void ScriptScanner::RegisterScript(ScriptInfo *info) +void ScriptScanner::RegisterScript(std::unique_ptr &&info) { std::string script_original_name = this->GetScriptName(*info); std::string script_name = fmt::format("{}.{}", script_original_name, info->GetVersion()); @@ -94,7 +91,6 @@ void ScriptScanner::RegisterScript(ScriptInfo *info) /* Check if GetShortName follows the rules */ if (info->GetShortName().size() != 4) { Debug(script, 0, "The script '{}' returned a string from GetShortName() which is not four characters. Unable to load the script.", info->GetName()); - delete info; return; } @@ -106,7 +102,6 @@ void ScriptScanner::RegisterScript(ScriptInfo *info) #else if (it->second->GetMainScript() == info->GetMainScript()) { #endif - delete info; return; } @@ -115,20 +110,20 @@ void ScriptScanner::RegisterScript(ScriptInfo *info) Debug(script, 1, " 2: {}", info->GetMainScript()); Debug(script, 1, "The first is taking precedence."); - delete info; return; } - this->info_list[script_name] = info; + ScriptInfo *script_info = this->info_vector.emplace_back(std::move(info)).get(); + this->info_list[script_name] = script_info; - if (!info->IsDeveloperOnly() || _settings_client.gui.ai_developer_tools) { + if (!script_info->IsDeveloperOnly() || _settings_client.gui.ai_developer_tools) { /* Add the script to the 'unique' script list, where only the highest version * of the script is registered. */ auto it = this->info_single_list.find(script_original_name); if (it == this->info_single_list.end()) { - this->info_single_list[script_original_name] = info; - } else if (it->second->GetVersion() < info->GetVersion()) { - it->second = info; + this->info_single_list[script_original_name] = script_info; + } else if (it->second->GetVersion() < script_info->GetVersion()) { + it->second = script_info; } } } diff --git a/src/script/script_scanner.hpp b/src/script/script_scanner.hpp index e70ffe7b0b..9bd49f3357 100644 --- a/src/script/script_scanner.hpp +++ b/src/script/script_scanner.hpp @@ -13,7 +13,7 @@ #include "../fileio_func.h" #include "../string_func.h" -typedef std::map ScriptInfoList; ///< Type for the list of scripts. +using ScriptInfoList = std::map; ///< Type for the list of scripts. /** Scanner to help finding scripts. */ class ScriptScanner : public FileScanner { @@ -51,7 +51,7 @@ public: /** * Register a ScriptInfo to the scanner. */ - void RegisterScript(class ScriptInfo *info); + void RegisterScript(std::unique_ptr &&info); /** * Get the list of registered scripts to print on the console. @@ -88,7 +88,9 @@ protected: std::string main_script; ///< The full path of the script. std::string tar_file; ///< If, which tar file the script was in. - ScriptInfoList info_list; ///< The list of all script. + std::vector> info_vector; + + ScriptInfoList info_list; ///< The list of all script. ScriptInfoList info_single_list; ///< The list of all unique script. The best script (highest version) is shown. /**