From 31fbb17c5b8996f96da59e44a4ddfa3ee620e4b5 Mon Sep 17 00:00:00 2001 From: glx22 Date: Tue, 3 Jun 2025 00:47:48 +0200 Subject: [PATCH] Codechange: Replace ScriptObject::[SG]etAllowDoCommand with ScriptObject::DisableDoCommandScope --- src/script/api/script_list.cpp | 7 +------ src/script/api/script_list.hpp | 12 +++--------- src/script/api/script_object.cpp | 14 ++++---------- src/script/api/script_object.hpp | 20 +++++--------------- src/script/script_instance.cpp | 32 +++++++++++++++----------------- 5 files changed, 28 insertions(+), 57 deletions(-) diff --git a/src/script/api/script_list.cpp b/src/script/api/script_list.cpp index 71e2a299a8..e7e2f9caff 100644 --- a/src/script/api/script_list.cpp +++ b/src/script/api/script_list.cpp @@ -910,8 +910,7 @@ SQInteger ScriptList::Valuate(HSQUIRRELVM vm) /* Don't allow docommand from a Valuator, as we can't resume in * mid C++-code. */ - bool backup_allow = ScriptObject::GetAllowDoCommand(); - ScriptObject::SetAllowDoCommand(false); + ScriptObject::DisableDoCommandScope disabler{}; /* Limit the total number of ops that can be consumed by a valuate operation */ SQOpsLimiter limiter(vm, MAX_VALUATE_OPS, "valuator function"); @@ -933,7 +932,6 @@ SQInteger ScriptList::Valuate(HSQUIRRELVM vm) /* Call the function. Squirrel pops all parameters and pushes the return value. */ if (SQ_FAILED(sq_call(vm, nparam + 1, SQTrue, SQFalse))) { - ScriptObject::SetAllowDoCommand(backup_allow); return SQ_ERROR; } @@ -956,7 +954,6 @@ SQInteger ScriptList::Valuate(HSQUIRRELVM vm) /* See below for explanation. The extra pop is the return value. */ sq_pop(vm, nparam + 4); - ScriptObject::SetAllowDoCommand(backup_allow); return sq_throwerror(vm, "return value of valuator is not valid (not integer/bool)"); } } @@ -966,7 +963,6 @@ SQInteger ScriptList::Valuate(HSQUIRRELVM vm) /* See below for explanation. The extra pop is the return value. */ sq_pop(vm, nparam + 4); - ScriptObject::SetAllowDoCommand(backup_allow); return sq_throwerror(vm, "modifying valuated list outside of valuator function"); } @@ -984,6 +980,5 @@ SQInteger ScriptList::Valuate(HSQUIRRELVM vm) * 4. The ScriptList instance object. */ sq_pop(vm, nparam + 3); - ScriptObject::SetAllowDoCommand(backup_allow); return 0; } diff --git a/src/script/api/script_list.hpp b/src/script/api/script_list.hpp index 67f9f92145..8a50c6d4e9 100644 --- a/src/script/api/script_list.hpp +++ b/src/script/api/script_list.hpp @@ -88,11 +88,9 @@ protected: sq_push(vm, 2); } - /* Don't allow docommand from a Valuator, as we can't resume in + /* Don't allow docommand from a filter, as we can't resume in * mid C++-code. */ - bool backup_allow = ScriptObject::GetAllowDoCommand(); - ScriptObject::SetAllowDoCommand(false); - + ScriptObject::DisableDoCommandScope disabler{}; if (nparam < 1) { ScriptList::FillList(list, item_valid); @@ -101,7 +99,7 @@ protected: SQOpsLimiter limiter(vm, MAX_VALUATE_OPS, "list filter function"); ScriptList::FillList(list, item_valid, - [vm, nparam, backup_allow](const T *item) { + [vm, nparam](const T *item) { /* Push the root table as instance object, this is what squirrel does for meta-functions. */ sq_pushroottable(vm); /* Push all arguments for the valuator function. */ @@ -112,7 +110,6 @@ protected: /* Call the function. Squirrel pops all parameters and pushes the return value. */ if (SQ_FAILED(sq_call(vm, nparam + 1, SQTrue, SQFalse))) { - ScriptObject::SetAllowDoCommand(backup_allow); throw static_cast(SQ_ERROR); } @@ -125,7 +122,6 @@ protected: break; default: - ScriptObject::SetAllowDoCommand(backup_allow); throw sq_throwerror(vm, "return value of filter is not valid (not bool)"); } @@ -139,8 +135,6 @@ protected: /* Pop the filter function */ sq_poptop(vm); } - - ScriptObject::SetAllowDoCommand(backup_allow); } template diff --git a/src/script/api/script_object.cpp b/src/script/api/script_object.cpp index c801449b5b..f82852db5c 100644 --- a/src/script/api/script_object.cpp +++ b/src/script/api/script_object.cpp @@ -63,6 +63,10 @@ ScriptObject::ActiveInstance::~ActiveInstance() ScriptObject::ActiveInstance::active = this->last_active; } +ScriptObject::DisableDoCommandScope::DisableDoCommandScope() + : AutoRestoreBackup(GetStorage()->allow_do_command, false) +{} + /* static */ ScriptInstance &ScriptObject::GetActiveInstance() { assert(ScriptObject::ActiveInstance::active != nullptr); @@ -205,16 +209,6 @@ ScriptObject::ActiveInstance::~ActiveInstance() return GetStorage()->last_cmd_ret; } -/* static */ void ScriptObject::SetAllowDoCommand(bool allow) -{ - GetStorage()->allow_do_command = allow; -} - -/* static */ bool ScriptObject::GetAllowDoCommand() -{ - return GetStorage()->allow_do_command; -} - /* static */ void ScriptObject::SetCompany(::CompanyID company) { if (GetStorage()->root_company == INVALID_OWNER) GetStorage()->root_company = company; diff --git a/src/script/api/script_object.hpp b/src/script/api/script_object.hpp index 6eef5537c8..710b0ce312 100644 --- a/src/script/api/script_object.hpp +++ b/src/script/api/script_object.hpp @@ -84,6 +84,11 @@ protected: static ScriptInstance *active; ///< The global current active instance. }; + class DisableDoCommandScope : private AutoRestoreBackup { + public: + DisableDoCommandScope(); + }; + /** * Save this object. * Must push 2 elements on the stack: @@ -265,21 +270,6 @@ protected: */ static const CommandDataBuffer &GetLastCommandResData(); - /** - * Store a allow_do_command per company. - * @param allow The new allow. - */ - static void SetAllowDoCommand(bool allow); - - /** - * Get the internal value of allow_do_command. This can differ - * from CanSuspend() if the reason we are not allowed - * to execute a DoCommand is in squirrel and not the API. - * In that case use this function to restore the previous value. - * @return True iff DoCommands are allowed in the current scope. - */ - static bool GetAllowDoCommand(); - /** * Set the current company to execute commands for or request * information about. diff --git a/src/script/script_instance.cpp b/src/script/script_instance.cpp index 5d86d1e971..68954b88e3 100644 --- a/src/script/script_instance.cpp +++ b/src/script/script_instance.cpp @@ -68,7 +68,7 @@ void ScriptInstance::Initialize(const std::string &main_script, const std::strin } try { - ScriptObject::SetAllowDoCommand(false); + ScriptObject::DisableDoCommandScope disabler{}; /* Load and execute the script for this script */ if (main_script == "%_dummy") { this->LoadDummyScript(); @@ -88,7 +88,6 @@ void ScriptInstance::Initialize(const std::string &main_script, const std::strin this->Died(); return; } - ScriptObject::SetAllowDoCommand(true); } catch (Script_FatalError &e) { this->is_dead = true; this->engine->ThrowError(e.GetErrorMessage()); @@ -213,21 +212,22 @@ void ScriptInstance::GameLoop() if (!this->is_started) { try { - ScriptObject::SetAllowDoCommand(false); - /* Run the constructor if it exists. Don't allow any DoCommands in it. */ - if (this->engine->MethodExists(*this->instance, "constructor")) { - if (!this->engine->CallMethod(*this->instance, "constructor", MAX_CONSTRUCTOR_OPS) || this->engine->IsSuspended()) { - if (this->engine->IsSuspended()) ScriptLog::Error("This script took too long to initialize. Script is not started."); + { + ScriptObject::DisableDoCommandScope disabler{}; + /* Run the constructor if it exists. Don't allow any DoCommands in it. */ + if (this->engine->MethodExists(*this->instance, "constructor")) { + if (!this->engine->CallMethod(*this->instance, "constructor", MAX_CONSTRUCTOR_OPS) || this->engine->IsSuspended()) { + if (this->engine->IsSuspended()) ScriptLog::Error("This script took too long to initialize. Script is not started."); + this->Died(); + return; + } + } + if (!this->CallLoad() || this->engine->IsSuspended()) { + if (this->engine->IsSuspended()) ScriptLog::Error("This script took too long in the Load function. Script is not started."); this->Died(); return; } } - if (!this->CallLoad() || this->engine->IsSuspended()) { - if (this->engine->IsSuspended()) ScriptLog::Error("This script took too long in the Load function. Script is not started."); - this->Died(); - return; - } - ScriptObject::SetAllowDoCommand(true); /* Start the script by calling Start() */ if (!this->engine->CallMethod(*this->instance, "Start", _settings_game.script.script_max_opcode_till_suspend) || !this->engine->IsSuspended()) this->Died(); } catch (Script_Suspend &e) { @@ -520,10 +520,9 @@ void ScriptInstance::Save() return; } else if (this->engine->MethodExists(*this->instance, "Save")) { HSQOBJECT savedata; - /* We don't want to be interrupted during the save function. */ - bool backup_allow = ScriptObject::GetAllowDoCommand(); - ScriptObject::SetAllowDoCommand(false); try { + /* We don't want to be interrupted during the save function. */ + ScriptObject::DisableDoCommandScope disabler{}; if (!this->engine->CallMethod(*this->instance, "Save", &savedata, MAX_SL_OPS)) { /* The script crashed in the Save function. We can't kill * it here, but do so in the next script tick. */ @@ -544,7 +543,6 @@ void ScriptInstance::Save() this->engine->CrashOccurred(); return; } - ScriptObject::SetAllowDoCommand(backup_allow); if (!sq_istable(savedata)) { ScriptLog::Error(this->engine->IsSuspended() ? "This script took too long to Save." : "Save function should return a table.");