Replaced bitmask-based argument validation system with more simple solution;

Refactor OpcodeContext::handleOpcode method;
Moved register_hook_* functions to new opcodeInfo table #58

sfall/Modules/Scripting/Handlers/Core.cpp

@@ -365,14 +365,6 @@ void sf_register_hook(OpcodeContext& ctx) {
 	RegisterHook(ctx.program(), id, proc);
 }
 
-void __declspec(naked) op_register_hook() {
-	_WRAP_OPCODE(sf_register_hook, 1, 0)
-}
-
-void __declspec(naked) op_register_hook_proc() {
-	_WRAP_OPCODE(sf_register_hook, 2, 0)
-}
-
 void __declspec(naked) op_sfall_ver_major() {
 	_OP_BEGIN(ebp)
 	__asm {

sfall/Modules/Scripting/Handlers/Core.h

@@ -49,10 +49,6 @@ void __declspec() op_set_self();
 // used for both register_hook and register_hook_proc
 void sf_register_hook(OpcodeContext&);
 
-void __declspec() op_register_hook();
-
-void __declspec() op_register_hook_proc();
-
 void __declspec() op_sfall_ver_major();
 
 void __declspec() op_sfall_ver_minor();

sfall/Modules/Scripting/Handlers/Metarule.cpp

@@ -20,6 +20,7 @@
 #include "..\..\ScriptExtender.h"
 #include "..\Arrays.h"
 #include "..\OpcodeContext.h"
+#include "..\OpcodeInfo.h"
 #include "AsmMacros.h"
 #include "Interface.h"
 #include "Misc.h"
@@ -37,12 +38,18 @@
 struct SfallMetarule {
 	// function name
 	const char* name;
+
 	// pointer to handler function
 	ScriptingFunctionHandler func;
-	// mininum number of arguments
+
+	// minimum number of arguments
 	int minArgs;
+
 	// maximum number of arguments
 	int maxArgs;
+	
+	// argument validation settings
+	OpcodeArgumentInfo argValidation[OP_MAX_ARGUMENTS];
 };
 
 typedef std::tr1::unordered_map<std::string, const SfallMetarule*> MetaruleTableType;
@@ -102,12 +109,13 @@ void sf_get_metarule_table(OpcodeContext& ctx) {
 		- name - name of function that will be used to call it from scripts,
 		- handler - pointer to handler function (see examples below),
 		- minArgs/maxArgs - minimum and maximum number of arguments allowed for this function
+		- argument types for validation
 */
 static const SfallMetarule metaruleArray[] = {
 	{"get_metarule_table", sf_get_metarule_table, 0, 0},
-	{"validate_test", sf_test, 2, 5},
-	{"spatial_radius", sf_spatial_radius, 1, 1},
-	{"critter_inven_obj2", sf_critter_inven_obj2, 2, 2},
+	{"validate_test", sf_test, 2, 5, {ARG_INT, ARG_FLOAT, ARG_STRING, ARG_ANY}},
+	{"spatial_radius", sf_spatial_radius, 1, 1, {ARG_OBJECT}},
+	{"critter_inven_obj2", sf_critter_inven_obj2, 2, 2, {ARG_OBJECT, ARG_INT}},
 	{"intface_redraw", sf_intface_redraw, 0, 0},
 	{"intface_show", sf_intface_show, 0, 0},
 	{"intface_hide", sf_intface_hide, 0, 0},
@@ -136,7 +144,7 @@ static bool ValidateMetaruleArguments(OpcodeContext& ctx, const SfallMetarule* m
 
 		return false;
 	} else {
-		return ctx.validateArguments(metaruleInfo->func);
+		return ctx.validateArguments(metaruleInfo->argValidation, metaruleInfo->name);
 	}
 }
 

sfall/Modules/Scripting/OpcodeContext.cpp

@@ -20,12 +20,10 @@
 
 #include "..\..\FalloutEngine\Fallout2.h"
 #include "..\ScriptExtender.h"
+#include "OpcodeInfo.h"
 
 #include "OpcodeContext.h"
 
-
-OpcodeMetadataMapType OpcodeContext::_opcodeMetaTable;
-
 OpcodeContext::OpcodeContext(TProgram* program, DWORD opcode, int argNum, bool hasReturn) {
 	assert(argNum < OP_MAX_ARGUMENTS);
 
@@ -92,22 +90,11 @@ void OpcodeContext::printOpcodeError(const char* fmt, ...) const {
 	Wrapper::debug_printf("\nOPCODE ERROR: %s\n   Current script: %s, procedure %s.", msg, _program->fileName, procName);
 }
 
-bool OpcodeContext::validateArguments(ScriptingFunctionHandler func) const {
-	OpcodeMetadataMapType::const_iterator it = _opcodeMetaTable.find(func);
-	if (it != _opcodeMetaTable.end()) {
-		const SfallOpcodeMetadata* meta = it->second;
-
-		// automatically validate argument types
-		return validateArguments(meta->argTypeMasks, meta->name);
-	}
-	return true;
-}
-
-bool OpcodeContext::validateArguments(const int argTypeMasks[], const char* opcodeName) const {
+bool OpcodeContext::validateArguments(const OpcodeArgumentInfo argInfo[], const char* opcodeName) const {
 	for (int i = 0; i < _numArgs; i++) {
-		int typeMask = argTypeMasks[i];
+		OpcodeArgumentInfo info = argInfo[i];
 		const ScriptValue &argI = arg(i);
-		if (typeMask != 0 && ((1 << argI.type()) & typeMask) == 0) {
+		if (info.type != DATATYPE_NONE && info.type != argI.type()) {
 			printOpcodeError(
 				"%s() - argument #%d has invalid type: %s.", 
 				opcodeName, 
@@ -115,7 +102,7 @@ bool OpcodeContext::validateArguments(const int argTypeMasks[], const char* opco
 				getSfallTypeName(argI.type()));
 
 			return false;
-		} else if ((typeMask & DATATYPE_MASK_NOT_NULL) && argI.rawValue() == 0) {
+		} else if (info.notNull && argI.rawValue() == 0) {
 			printOpcodeError(
 				"%s() - argument #%d is null.", 
 				opcodeName, 
@@ -128,39 +115,21 @@ bool OpcodeContext::validateArguments(const int argTypeMasks[], const char* opco
 }
 
 void OpcodeContext::handleOpcode(ScriptingFunctionHandler func) {
-	// process arguments on stack (reverse order)
-	for (int i = _numArgs - 1; i >= 0; i--) {
-		// get argument from stack
-		DWORD rawValueType = Wrapper::interpretPopShort(_program);
-		DWORD rawValue = Wrapper::interpretPopLong(_program);
-		SfallDataType type = static_cast<SfallDataType>(getSfallTypeByScriptType(rawValueType));
+	_popArguments();
 
-		// retrieve string argument
-		if (type == DATATYPE_STR) {
-			_args.at(i) = Wrapper::interpretGetString(_program, rawValueType, rawValue);
-		} else {
-			_args.at(i) = ScriptValue(type, rawValue);
-		}
-	}
+	func(*this);
 
-	// call opcode handler if arguments are valid (or no automatic validation was done)
-	if (validateArguments(func)) {
+	_pushReturnValue();
+}
+
+void OpcodeContext::handleOpcode(ScriptingFunctionHandler func, const OpcodeArgumentInfo argTypes[], const char* opcodeName) {
+	_popArguments();
+
+	if (validateArguments(argTypes, opcodeName)) {
 		func(*this);
 	}
 
-	// process return value
-	if (_hasReturn) {
-		if (_ret.type() == DATATYPE_NONE) {
-			// if no value was set in handler, force return 0 to avoid stack error
-			_ret = ScriptValue(0);
-		}
-		DWORD rawResult = _ret.rawValue();
-		if (_ret.type() == DATATYPE_STR) {
-			rawResult = Wrapper::interpretAddString(_program, _ret.asString());
-		}
-		Wrapper::interpretPushLong(_program, rawResult);
-		Wrapper::interpretPushShort(_program, getScriptTypeBySfallType(_ret.type()));
-	}
+	_pushReturnValue();
 }
 
 void __stdcall OpcodeContext::handleOpcodeStatic(TProgram* program, DWORD opcodeOffset, ScriptingFunctionHandler func, int argNum, bool hasReturn) {
@@ -170,10 +139,6 @@ void __stdcall OpcodeContext::handleOpcodeStatic(TProgram* program, DWORD opcode
 	currentContext.handleOpcode(func);
 }
 
-void OpcodeContext::addOpcodeMetaData(const SfallOpcodeMetadata* data) {
-	_opcodeMetaTable[data->handler] = data;
-}
-
 const char* OpcodeContext::getSfallTypeName(DWORD dataType) {
 	switch (dataType) {
 		case DATATYPE_NONE:
@@ -214,3 +179,35 @@ DWORD OpcodeContext::getScriptTypeBySfallType(DWORD dataType) {
 			return VAR_TYPE_INT;
 	}
 }
+
+void OpcodeContext::_popArguments() {	
+	// process arguments on stack (reverse order)
+	for (int i = _numArgs - 1; i >= 0; i--) {
+		// get argument from stack
+		DWORD rawValueType = Wrapper::interpretPopShort(_program);
+		DWORD rawValue = Wrapper::interpretPopLong(_program);
+		SfallDataType type = static_cast<SfallDataType>(getSfallTypeByScriptType(rawValueType));
+
+		// retrieve string argument
+		if (type == DATATYPE_STR) {
+			_args.at(i) = Wrapper::interpretGetString(_program, rawValueType, rawValue);
+		} else {
+			_args.at(i) = ScriptValue(type, rawValue);
+		}
+	}
+}
+
+void OpcodeContext::_pushReturnValue() {
+	if (_hasReturn) {
+		if (_ret.type() == DATATYPE_NONE) {
+			// if no value was set in handler, force return 0 to avoid stack error
+			_ret = ScriptValue(0);
+		}
+		DWORD rawResult = _ret.rawValue();
+		if (_ret.type() == DATATYPE_STR) {
+			rawResult = Wrapper::interpretAddString(_program, _ret.asString());
+		}
+		Wrapper::interpretPushLong(_program, rawResult);
+		Wrapper::interpretPushShort(_program, getScriptTypeBySfallType(_ret.type()));
+	}
+}

sfall/Modules/Scripting/OpcodeContext.h

@@ -27,30 +27,11 @@
 // variables for new opcodes
 #define OP_MAX_ARGUMENTS	(10)
 
-// masks for argument validation
-#define DATATYPE_MASK_INT		(1 << DATATYPE_INT)
-#define DATATYPE_MASK_FLOAT		(1 << DATATYPE_FLOAT)
-#define DATATYPE_MASK_STR		(1 << DATATYPE_STR)
-#define DATATYPE_MASK_NOT_NULL	(0x00010000)
-#define DATATYPE_MASK_VALID_OBJ	(DATATYPE_MASK_INT | DATATYPE_MASK_NOT_NULL)
-
 class OpcodeContext;
+struct OpcodeArgumentInfo;
 
 typedef void(*ScriptingFunctionHandler)(OpcodeContext&);
 
-struct SfallOpcodeMetadata {
-	// opcode handler, will be used as key
-	ScriptingFunctionHandler handler;
-
-	// opcode name, only used for logging
-	const char* name;
-
-	// argument validation masks
-	int argTypeMasks[OP_MAX_ARGUMENTS];
-};
-
-typedef std::tr1::unordered_map<ScriptingFunctionHandler, const SfallOpcodeMetadata*> OpcodeMetadataMapType;
-
 // A context for handling opcodes. Opcode handlers can retrieve arguments and set opcode return value via context.
 class OpcodeContext {
 public:
@@ -94,18 +75,19 @@ public:
 	// writes error message to debug.log along with the name of script & procedure
 	void printOpcodeError(const char* fmt, ...) const;
 
-	// Validate opcode arguments against type masks
+	// Validate opcode arguments against type specification
 	// if validation pass, returns true, otherwise writes error to debug.log and returns false
-	bool validateArguments(const int argTypeMasks[], const char* opcodeName) const;
-
-	// validate opcode arguments given opcode handler function and actual number of arguments
-	bool validateArguments(ScriptingFunctionHandler func) const;
+	bool validateArguments(const OpcodeArgumentInfo argInfo[], const char* opcodeName) const;
 
 	// Handle opcodes
 	// func - opcode handler
 	void handleOpcode(ScriptingFunctionHandler func);
 
-	static void addOpcodeMetaData(const SfallOpcodeMetadata* data);
+	// Handle opcodes with argument validation
+	// func - opcode handler
+	// argTypes - argument types for validation
+	// opcodeName - name of a function (for logging)
+	void handleOpcode(ScriptingFunctionHandler func, const OpcodeArgumentInfo argTypes[], const char* opcodeName);
 
 	// handles opcode using default instance
 	static void __stdcall handleOpcodeStatic(TProgram* program, DWORD opcodeOffset, ScriptingFunctionHandler func, int argNum, bool hasReturn);
@@ -117,6 +99,11 @@ public:
 	static DWORD getScriptTypeBySfallType(DWORD dataType);
 
 private:
+	// pops arguments from data stack
+	void _popArguments();
+	// pushes return value to data stack
+	void _pushReturnValue();
+
 	TProgram* _program;
 	DWORD _opcode;
 
@@ -125,6 +112,4 @@ private:
 	int _argShift;
 	std::array<ScriptValue, OP_MAX_ARGUMENTS> _args;
 	ScriptValue _ret;
-
-	static OpcodeMetadataMapType _opcodeMetaTable;
 };

sfall/Modules/Scripting/OpcodeInfo.h

@@ -0,0 +1,36 @@
+#pragma once
+
+#include "ScriptValue.h"
+
+typedef struct OpcodeArgumentInfo {
+	// the type of argument, NONE means any type
+	SfallDataType type;
+	// set true to check for null (0) value - useful for objects, arrays, etc.
+	bool notNull;
+} OpcodeArgumentInfo;
+
+typedef struct SfallOpcodeInfo {
+	// opcode number
+	int opcode;
+
+	// opcode name
+	const char name[32];
+
+	// opcode handler
+	ScriptingFunctionHandler handler;
+
+	// number of arguments
+	int argNum;
+
+	// has return value or not
+	bool hasReturn;
+
+	// argument validation settings
+	OpcodeArgumentInfo argValidation[OP_MAX_ARGUMENTS];
+} SfallOpcodeInfo;
+
+#define ARG_ANY		{DATATYPE_NONE}
+#define ARG_INT		{DATATYPE_INT}
+#define ARG_FLOAT	{DATATYPE_FLOAT}
+#define ARG_STRING	{DATATYPE_STR}
+#define ARG_OBJECT	{DATATYPE_INT, true}

sfall/Modules/Scripting/Opcodes.cpp

@@ -36,6 +36,7 @@
 #include "Handlers\Worldmap.h"
 #include "Handlers\Metarule.h"
 #include "OpcodeContext.h"
+#include "OpcodeInfo.h"
 
 #include "Opcodes.h"
 
@@ -44,26 +45,6 @@ static const short opcodeCount = 0x300;
 
 static void* opcodes[opcodeCount];
 
-typedef struct SfallOpcodeInfo {
-	// opcode number
-	int opcode;
-
-	// opcode name
-	const char name[32];
-
-	// opcode handler
-	ScriptingFunctionHandler handler;
-
-	// number of arguments
-	int argNum;
-
-	// has return value or not
-	bool hasReturn;
-
-	// argument validation masks
-	int argTypeMasks[OP_MAX_ARGUMENTS];
-} SfallOpcodeInfo;
-
 typedef std::tr1::unordered_map<int, const SfallOpcodeInfo*> OpcodeInfoMapType;
 
 // Opcode Table. Add additional (sfall) opcodes here.
@@ -73,16 +54,18 @@ typedef std::tr1::unordered_map<int, const SfallOpcodeInfo*> OpcodeInfoMapType;
 //    function handler,
 //    number of arguments,
 //    has return value,
-//    type masks for each argument (for validation)
+//    { argument 1 type, argument 2 type, ...}
 // }
 static SfallOpcodeInfo opcodeInfoArray[] = {
 	{0x16c, "key_pressed", sf_key_pressed, 1, true},
 	{0x1f5, "get_script", sf_get_script, 1, true},
+	{0x207, "register_hook", sf_register_hook, 1, false, {ARG_INT}},
 	{0x216, "set_critter_burst_disable", sf_set_critter_burst_disable, 2, false},
-	{0x217, "get_weapon_ammo_pid", sf_get_weapon_ammo_pid, 1, true},
-	{0x218, "set_weapon_ammo_pid", sf_set_weapon_ammo_pid, 2, false, {DATATYPE_INT, DATATYPE_INT}},
-	{0x219, "get_weapon_ammo_count", sf_get_weapon_ammo_count, 1, true},
-	{0x21a, "set_weapon_ammo_count", sf_set_weapon_ammo_count, 2, false, {DATATYPE_INT, DATATYPE_INT}},
+	{0x217, "get_weapon_ammo_pid", sf_get_weapon_ammo_pid, 1, true, {ARG_OBJECT}},
+	{0x218, "set_weapon_ammo_pid", sf_set_weapon_ammo_pid, 2, false, {ARG_OBJECT, ARG_INT}},
+	{0x219, "get_weapon_ammo_count", sf_get_weapon_ammo_count, 1, true, {ARG_OBJECT}},
+	{0x21a, "set_weapon_ammo_count", sf_set_weapon_ammo_count, 2, false, {ARG_OBJECT, ARG_INT}},
+	{0x262, "register_hook_proc", sf_register_hook, 2, false, {ARG_INT, ARG_INT}},
 	{0x26e, "obj_blocking_line", sf_make_straight_path, 3, true},
 	{0x26f, "obj_blocking_tile", sf_obj_blocking_at, 3, true},
 	{0x270, "tile_get_objs", sf_tile_get_objects, 2, true},
@@ -104,23 +87,6 @@ static SfallOpcodeInfo opcodeInfoArray[] = {
 // initialized at run time from the array above
 OpcodeInfoMapType opcodeInfoMap;
 
-/*
-	Array for opcodes metadata.
-
-	This is completely optional, added for convenience only.
-
-	By adding opcode to this array, Sfall will automatically validate it's arguments using provided info.
-	On fail, errors will be printed to debug.log and opcode will not be executed.
-	If you don't include opcode in this array, you should take care of all argument validation inside handler itself.
-*/
-static const SfallOpcodeMetadata opcodeMetaArray[] = {
-	{sf_register_hook, "register_hook[_proc]", {DATATYPE_MASK_INT, DATATYPE_MASK_INT}},
-	{sf_test, "validate_test", {DATATYPE_MASK_INT, DATATYPE_MASK_INT | DATATYPE_MASK_FLOAT, DATATYPE_MASK_STR, DATATYPE_NONE}},
-	{sf_spatial_radius, "spatial_radius", {DATATYPE_MASK_VALID_OBJ}},
-	{sf_critter_inven_obj2, "critter_inven_obj2", {DATATYPE_MASK_VALID_OBJ, DATATYPE_MASK_INT}},
-	//{op_message_str_game, {}}
-};
-
 // Initializes the opcode info table.
 void InitOpcodeInfoTable() {
 	int length = sizeof(opcodeInfoArray) / sizeof(opcodeInfoArray[0]);
@@ -129,13 +95,6 @@ void InitOpcodeInfoTable() {
 	}
 }
 
-void InitOpcodeMetaTable() {
-	int length = sizeof(opcodeMetaArray) / sizeof(opcodeMetaArray[0]);
-	for (int i = 0; i < length; ++i) {
-		OpcodeContext::addOpcodeMetaData(&opcodeMetaArray[i]);
-	}
-}
-
 // Default handler for Sfall Opcodes. 
 // Searches current opcode in Opcode Info table and executes the appropriate handler.
 void __stdcall defaultOpcodeHandlerStdcall(TProgram* program, DWORD opcodeOffset) {
@@ -144,7 +103,7 @@ void __stdcall defaultOpcodeHandlerStdcall(TProgram* program, DWORD opcodeOffset
 	if (iter != opcodeInfoMap.end()) {
 		auto info = iter->second;
 		OpcodeContext ctx(program, opcode, info->argNum, info->hasReturn);
-		ctx.handleOpcode(info->handler);
+		ctx.handleOpcode(info->handler, info->argValidation, info->name);
 	} else {
 		Wrapper::interpretError("Unknown opcode: %d", opcode);
 	}
@@ -343,7 +302,6 @@ void InitNewOpcodes() {
 	opcodes[0x204] = op_get_proto_data;
 	opcodes[0x205] = op_set_proto_data;
 	opcodes[0x206] = op_set_self;
-	opcodes[0x207] = op_register_hook;
 	opcodes[0x208] = op_fs_write_bstring;
 	opcodes[0x209] = op_fs_read_byte;
 	opcodes[0x20a] = op_fs_read_short;
@@ -431,7 +389,6 @@ void InitNewOpcodes() {
 	opcodes[0x25f] = op_reg_anim_take_out;
 	opcodes[0x260] = op_reg_anim_turn_towards;
 	opcodes[0x261] = op_explosions_metarule;
-	opcodes[0x262] = op_register_hook_proc;
 	opcodes[0x263] = op_power;
 	opcodes[0x264] = op_log;
 	opcodes[0x265] = op_exponent;
@@ -454,6 +411,5 @@ void InitNewOpcodes() {
 	// see opcodeMetaArray above for additional scripting functions via "metarule"
 
 	InitOpcodeInfoTable();
-	InitOpcodeMetaTable();
 	InitMetaruleTable();
 }

sfall/ddraw.vcxproj

@@ -239,6 +239,7 @@
     <ClInclude Include="Modules\Scripting\ScriptValue.h" />
     <ClInclude Include="Modules\Scripting\Handlers\Stats.h" />
     <ClInclude Include="Modules\Scripting\Handlers\Worldmap.h" />
+    <ClInclude Include="Modules\Scripting\OpcodeInfo.h" />
     <ClInclude Include="win9x\Cpp11_emu.h" />
     <ClInclude Include="Modules\CRC.h" />
     <ClInclude Include="Modules\Credits.h" />

sfall/ddraw.vcxproj.filters

@@ -223,6 +223,9 @@
     <ClInclude Include="Modules\Scripting\Opcodes.h">
       <Filter>Modules\Scripting</Filter>
     </ClInclude>
+    <ClInclude Include="Modules\Scripting\OpcodeInfo.h">
+      <Filter>Modules\Scripting</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <ClCompile Include="main.cpp" />