Codechange: Replace ScriptObject::[SG]etAllowDoCommand with ScriptObject::DisableDoCommandScope

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::DisableDoCommandScope disabler{};
-	ScriptObject::SetAllowDoCommand(false);
 
 	/* 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;
 }

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::DisableDoCommandScope disabler{};
-		ScriptObject::SetAllowDoCommand(false);
-
 
 		if (nparam < 1) {
 			ScriptList::FillList<T>(list, item_valid);
@@ -101,7 +99,7 @@ protected:
 			SQOpsLimiter limiter(vm, MAX_VALUATE_OPS, "list filter function");
 
 			ScriptList::FillList<T>(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<SQInteger>(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 <typename T>

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;

src/script/api/script_object.hpp

@@ -84,6 +84,11 @@ protected:
 		static ScriptInstance *active;  ///< The global current active instance.
 	};
 
+	class DisableDoCommandScope : private AutoRestoreBackup<bool> {
+	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.

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. */
+				ScriptObject::DisableDoCommandScope disabler{};
-			if (this->engine->MethodExists(*this->instance, "constructor")) {
+				/* Run the constructor if it exists. Don't allow any DoCommands in it. */
-				if (!this->engine->CallMethod(*this->instance, "constructor", MAX_CONSTRUCTOR_OPS) || this->engine->IsSuspended()) {
+				if (this->engine->MethodExists(*this->instance, "constructor")) {
-					if (this->engine->IsSuspended()) ScriptLog::Error("This script took too long to initialize. Script is not started.");
+					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.");