Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed 0AB1 input argument count verification for string arrays. #50

Merged
merged 3 commits into from
Nov 19, 2023

Conversation

MiranDMC
Copy link
Collaborator

Enabled using string array argument as module name in 0AB1.

…ed using string array argument as module name in 0AB1.
@MiranDMC MiranDMC requested a review from x87 November 18, 2023 20:34
return false;
}
static bool IsString(eDataType type)
{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why IsString includes variables and IsInteger does not? IsVariable should be part of IsInteger

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there is no DT_VAR_INT.
DT_VAR actually can contain multiple types.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe if you call it IsNumber then it would better reflect what it does

Copy link
Collaborator Author

@MiranDMC MiranDMC Nov 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How should it be called then? DT_VAR can in fact be string too (pointer to text).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make 2 functions: IsNumber and IsString.

If IsNumber returns true, it's a label, one path. If IsString, it's a module, another path. Otherwise, error

Copy link
Collaborator Author

@MiranDMC MiranDMC Nov 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions reflect type to text function

static const char* ToKindStr(eDataType type)
{
	switch (type)
	{
		case DT_BYTE:
		case DT_WORD:
		case DT_DWORD:
			return "int"; break;

		case DT_FLOAT:
			return "float"; break;

		case DT_STRING:
		case DT_TEXTLABEL:
		case DT_LVAR_TEXTLABEL:
		case DT_LVAR_TEXTLABEL_ARRAY:
		case DT_LVAR_STRING:
		case DT_LVAR_STRING_ARRAY:
		case DT_VAR_TEXTLABEL:
		case DT_VAR_TEXTLABEL_ARRAY:
		case DT_VAR_STRING:
		case DT_VAR_STRING_ARRAY:
		case DT_VARLEN_STRING:
			return "string"; break;

		case DT_VAR:
		case DT_VAR_ARRAY:
		case DT_LVAR:
		case DT_LVAR_ARRAY:
			return "variable"; break;

		case DT_END:
			return "varArgEnd"; break;

		default:
			return "corrupted"; break;
	}
}

Motivation was to just make simpler verification of DATA TYPE byte without need for switches, where as already proved few times, it is to easy to forget to include some cases.

These utilities are meant to verify eDataType sub-category only, in abstraction of actual in code use (as mentioned it would be possible to deduce int-float value type within arrays).

It would perhaps make more sense if it was DataType::IsInteger(type), but that would require C++ features set.

Copy link

@x87 x87 Nov 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't get why we separate int literal and int variable while combine string literal and string variable. should be some consistency at least. we either combine literals and variables (for int, float, and string types) under the same check, or separate them out (for all three types).

In some cases, like mentioned 0AB1's arguments count which is compile constant too, so makes no sense to be variable.

there is nothing that prevents you to use a variable in this position, though.

Copy link
Collaborator Author

@MiranDMC MiranDMC Nov 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned because there is DT_LVAR_STRING but no DT_LVAR_INT or DT_LVAR_FLOAT. I just organized DT_x types together in groups of same flavour.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm splitting then the functions to separate immediate and variable values.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed!

@MiranDMC MiranDMC merged commit 5035fda into master Nov 19, 2023
@MiranDMC MiranDMC deleted the fix_argument_count_validation_in_0AB1 branch November 19, 2023 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants