Added some new string manipulation stocks #1224

milutinke · 2020-04-07T13:03:16Z

Added the following stocks:

IsStringNumber
IsStringFloat
StringToLowerCase
StringToUpperCase
IndexOfChar
LastIndexOfChar
CopyC

Tested:
Test plugin source link: https://pastebin.com/hqfSZNxY
Test results: https://i.imgur.com/4CawNKq.png

Added the following stocks: - IsStringNumber - IsStringFloat - StringToLowerCase - StringToUpperCase - IndexOfChar - LastIndexOfChar Tested: Test plugin source link: https://pastebin.com/QWXUy0ta Test results: https://i.imgur.com/NKhvsl4.png

dvander · 2020-04-07T15:21:35Z

plugins/include/string.inc

+ * @param szString     Input string
+ * @return             Boolean
+ */
+stock bool IsStringNumber(const char[] szString) 


Do you need a temporary string? I think it would work to skip past spaces and then call StringToIntExt() and check the return value.

I never heard of StringToIntExt and I cannot find it in the documentation.
The reason I used a temporary value is to avoid modifying the original string when trimming.

Sorry, StringToIntEx. You don't need to modify the string if you skip spaces with a counter.

However, I tried with StringToInt and StringToFloat and it did not work properly, the test which needed to pass, have failed.

Results of those functions: https://i.imgur.com/4l1JzGd.png

dvander · 2020-04-07T15:23:31Z

plugins/include/string.inc

+{
+	int iIterator = 0;
+	int iLength = strlen(szInput);
+	char[] szOutput = new char[iLength + 1];


Is a temporary needed here, given that the string is modified in place?

Yeah, they are not needed. I am going to correct it.

dvander · 2020-04-07T15:26:30Z

plugins/include/string.inc

+{
+	int iIterator = 0;
+	int iLength = strlen(szInput);
+	char[] szOutput = new char[iLength + 1];


Same comment as above.

dvander · 2020-04-07T15:27:24Z

plugins/include/string.inc

+	int iP;
+	int iLength = strlen(szString);
+
+	while (0 < szString[iIterator] <= 255) {


It will be much easier to write this as a wrapper around StringToFloatEx(), which will also make sure the parsing is consistent.

Removed unnecessary temporary variables.

KyleSanderson

I've rolled these myself, but they can definitely be simpler and wrapped around our natives. so, + dvander's comments we already have some of this in the tree.

plugins/include/string.inc

milutinke · 2020-04-10T10:07:25Z

I have simplified the StringToUpper and StringToLower case.
Regarding the FindCharInString, I did not know about that function, I can change IndexOfChar and LastIndexOfChar to wrap the FindCharInString.
IsStringNumber and IsStringFloat do not work intended when they wrap StringToInt and StringToFloat. Results of the test with those: https://i.imgur.com/4l1JzGd.png

peace-maker · 2020-04-10T10:41:13Z

That's why dvander suggested using the Ex variants of those natives. They return the number of characters consumed for the number, so checking if the whole string was consumed does what you want.

stock bool IsStringInteger(const char[] str, int nBase=10)
{
	int result;
	return StringToIntEx(str, result, nBase) == strlen(str);
}

stock bool IsStringFloat(const char[] str)
{
	float result;
	return StringToFloatEx(str, result) == strlen(str);
}

milutinke

That's why dvander suggested using the Ex variants of those natives. They return the number of characters consumed for the number, so checking if the whole string was consumed does what you want.
stock bool IsStringInteger(const char[] str, int nBase=10)
{
	int result;
	return StringToIntEx(str, result, nBase) == strlen(str);
}

stock bool IsStringFloat(const char[] str)
{
	float result;
	return StringToFloatEx(str, result) == strlen(str);
}

Yeah, but IsStringFloat fails and counts -1 and -3 as floats, and they are not.
Edit: Nevermind, fixed:

stock bool IsStringFloat(const char[] str)
{
	float result;
	return StringToFloatEx(str, result) == strlen(str) && FindCharInString(str, '.') != -1;
}

milutinke · 2020-04-07T16:02:50Z

plugins/include/string.inc

+ * @param szString     Input string
+ * @return             Boolean
+ */
+stock bool IsStringNumber(const char[] szString) 


However, I tried with StringToInt and StringToFloat and it did not work properly, the test which needed to pass, have failed.

milutinke · 2020-04-07T16:03:38Z

plugins/include/string.inc

+{
+	int iIterator = 0;
+	int iLength = strlen(szInput);
+	char[] szOutput = new char[iLength + 1];


Yeah, they are not needed. I am going to correct it.

milutinke · 2020-04-07T16:13:51Z

plugins/include/string.inc

+ * @param szString     Input string
+ * @return             Boolean
+ */
+stock bool IsStringNumber(const char[] szString) 


Results of those functions: https://i.imgur.com/4l1JzGd.png

milutinke · 2020-04-07T16:17:19Z

plugins/include/string.inc

+{
+	int iIterator = 0;
+	int iLength = strlen(szInput);
+	char[] szOutput = new char[iLength + 1];


dvander · 2020-04-10T22:58:27Z

If you want to be very strict, you can do:

stock bool IsStringFloat(const char[] str)
{
	float result;
	return StringToFloatEx(str, result) == strlen(str) && !IsStringInt(str);
}

But I'm not sure I'm in favor of this. Integers are a subset of floats, and it's kind of arbitrary to demand that decimals are added for whole numbers.

Changed IsStringNumber and IsStringFloat to wrap StringToIntEx and StringToFloatEx. Changed IndexOfChar to wrap FindCharInString. Added CopyC

milutinke · 2020-04-10T23:06:32Z

Changed the code to apply your advices.

Changed IsStringNumber and IsStringFloat to wrap StringToIntEx and StringToFloatEx.
Changed IndexOfChar to wrap FindCharInString.
Added CopyC

Tested, everything works:
Test plugin source link: https://pastebin.com/hqfSZNxY
Test results: https://i.imgur.com/4CawNKq.png

If the string has a floating point value or not, we can still store it in a float. No need to check the additional stocks, they're built-in. CopyC already exists, that's simply strcopy.

KyleSanderson · 2020-08-16T23:42:46Z

@milutinke I've made some adjustments that I thought were appropriate. What do you think?

KyleSanderson · 2020-10-03T00:45:42Z

@milutinke any update on this?

milutinke · 2021-05-21T10:53:28Z

@milutinke any update on this?

Sorry for replying this late, I've not received a notification about this for some reason back then.
CopyC does not exist in Source Mod, at least to my knowledge.
Also, I think that return StringToFloatEx(str, result) == strlen(str); will not work properly in IsStringFloat without && FindCharInString(str, '.') != -1; if I remember correctly.

sirdigbot · 2021-12-26T10:17:24Z

If the goal is to add more string stocks for common stuff, I feel like borrowing a few more ideas from python would make sense.
Such as IsStringAlpha/Numeric/AlNum, IsStringASCII, IsStringPrintable, StringEndsWith.
There's already char versions of some of those, so making string equivalents doesn't seem unreasonable.

Alienmario · 2022-01-15T20:54:40Z

If the goal is to add more string stocks for common stuff, I feel like borrowing a few more ideas from python would make sense. Such as IsStringAlpha/Numeric/AlNum, IsStringASCII, IsStringPrintable, StringEndsWith. There's already char versions of some of those, so making string equivalents doesn't seem unreasonable.

+1 I've missed StringEndsWith in almost every other plugin I made.

Alienmario · 2023-01-30T18:14:19Z

Sorry to bump, can we get some update on this? I feel like converting String case is a very fundamental operation and it would be nice to have SM ship with it.

peace-maker · 2023-07-03T19:12:32Z

Would adding wrappers for other available stock functions really help or make the API more confusing?

I've reviewed this again and am in favor of adding more string utility stocks as suggested, but am hesitant on adding wrappers like IndexOfChar -> FindCharInString. Having multiple functions doing the same thing doesn't help readability of code. But FindCharInString is a mouthful and not the best choice of name for what it does after all.

milutinke · 2023-07-03T20:39:25Z

Would adding wrappers for other available stock functions really help or make the API more confusing?

I've reviewed this again and am in favor of adding more string utility stocks as suggested, but am hesitant on adding wrappers like IndexOfChar -> FindCharInString. Having multiple functions doing the same thing doesn't help readability of code. But FindCharInString is a mouthful and not the best choice of name for what it does after all.

People who come from other programming languages are used to IndexOfChar name, so it would not hurt having it in my opinion.
But yeah, I am fine without it being added too.

sirdigbot · 2023-07-05T02:14:11Z

I have an assortment of issues with the current functions, but I'm told I should put the improvements here instead of making a competing PR.
Sorry in advance.

Bug

IsStringNumber/IsStringFloat both incorrectly consider an empty string as valid.

Naming

IsStringNumber only detects integers so it should be called IsStringInteger
IsStringFloat is misleading. As dvander mentioned it should (and does) handle integer values too (usually what's important is "can it convert to a float"). IsStringDecimal would be better as StringToFloat requires base-10 numbers anyway.
StringToLowerCase should be StringToLower both because it's shorter and because it matches IsCharLower and CharToLower
Ditto for StringToUpperCase

Style

Hungarian notation (prefixing with the type) should be removed from params and names should be made more consistent with the other functions. e.g. szInput -> str
Variables also should not use hungarian notation
EOS is never used anywhere in sourcemod's codebase except a single test case. I'm not sure if it's considered bad to use compared to '\0' but it might be.
Both the StringToLower/Upper functions could've just been for-loops for readability sake: for (int i = 0; str[i] != '\0'; ++i) is a common idiom.
In IndexOfChar's docstring, "Do we need the last index (Optional, default: false)" should probably borrow the description from FindCharInString's parameter: "False (default) to search forward, true to search backward."

Misc

IsStringNumber is missing the nBase parameter in its docstring
Docstrings have incorrect @return descriptions--the meaning of the return values (not the type) is supposed to be there. So for IsStringFloat for example, @return Boolean should be changed to something like @return True if string is a valid float number.
@return is not needed for void functions.
"Input string for conversion and also the output" is unclear and wordy. You could borrow from CharToLower/Upper: "Converts each character in a string to its lowercase counterpart."
"Supports negative values" should not need to be clarified for IsStringNumber/IsStringFloat since not supporting them is incorrect.
The docstrings for every single function have the incorrect parameter names for every parameter.
For IndexOfChar, "index of the found character" is unclear because it actually finds the first occurrence of the character.
(Picky) const char cCharacter doesn't really need to be const since scalar values are always passed by-value (i.e. copied) anyway.

Alienmario · 2024-06-22T10:44:28Z

@sirdigbot Maybe you should make a PR after all, as this one appears cursed.
I can help providing some stocks I've been using.

sirdigbot · 2024-06-28T12:23:56Z

@sirdigbot Maybe you should make a PR after all, as this one appears cursed. I can help providing some stocks I've been using.

Sure, go ahead. I'll take a look. Though I already have a massive list of proposed stocks from the last time I had a go at a PR.

milutinke · 2024-06-28T13:13:03Z

For some reason I did not get notifications until now (all of them) for this PR even I'm subscribed.
Since I've stopped doing any CS:GO related stuff years ago, I'll close the PR, thus a new one can be opened with the proposed changes.

Alienmario · 2024-06-28T15:27:07Z

Sure, go ahead. I'll take a look. Though I already have a massive list of proposed stocks from the last time I had a go at a PR.

stock bool StrEndsWith(const char[] str, const char[] suffix, bool caseSensitive = true)
{
	return StrEndsWithEx(str, strlen(str), suffix, caseSensitive);
}

stock bool StrEndsWithEx(const char[] str, int realLen, const char[] suffix, bool caseSensitive = true)
{
	int suffixLen = strlen(suffix);
	return (realLen >= suffixLen && StrEqual(str[realLen - suffixLen], suffix, caseSensitive));
}

// Non-ex func already used by arraylist
stock int FindStringInArrayEx(const char[][] array, int len, const char[] str, bool caseSensitive = true)
{
	for (int i = 0; i < len; i++)
	{
		if (StrEqual(str, array[i], caseSensitive))
		{
			return i;
		}
	}
	return -1;
}

// Non-ex funcs already used by arraylist
stock int FindValueInArrayEx(const any[] array, int len, const any val)
{
	for (int i = 0; i < len; i++)
	{
		if (array[i] == val)
		{
			return i;
		}
	}
	return -1;
}

stock int FindCharInArray(const any[] array, int len, const int val, bool caseSensitive = true)
{
	int i = FindValueInArrayEx(array, len, val);
	if (!caseSensitive && i == -1)
	{
		int val2 = IsCharLower(val) ? CharToUpper(val) : CharToLower(val);
		if (val2 != val)
		{
			i = FindValueInArrayEx(array, len, val2);
		}
	}
	return i;
}

// Like SplitString, but more efficient?
stock int SplitByChar(const char[] szSource, char c, char[] szBuffer, int nSize)
{
	int i = 0;
	while (szSource[i] != c)
	{
		if (szSource[i] == '\0')
		{
			return -1;
		}
		i++;
	}
	i++;
	strcopy(szBuffer, i < nSize ? i : nSize, szSource);
	return i;
}

If you pick any, I can clean them up and add docs. I especially like StrEndsWithEx as it lets me check for multiple file extensions without calculating string length repeatedly.

Additionally I think FindCharInString stock can be optimized (in forward mode at least) by removing strlen at the beginning and checking for end of string inside the loop.

sirdigbot · 2024-06-29T00:16:54Z

StringEndsWith is already on my big list, as is StringStartsWith
StrEndsWithEx I feel is too specific and your use case is better replaced by regex.
FindStringInArrayEx is alright, but it might not be enough to justify a stock. I'll add it to my list anyway and let the maintainers decide.
FindValueInArrayEx and FindCharInArray aren't really string related as far as I can tell. Not sure of their use case.
SplitByChar isn't needed. SplitString may not be the perfectly efficient but the stocks are more meant for handling common operations more than micro-optimising stuff. At least in my opinion

Alienmario · 2024-06-29T10:00:45Z

StrEndsWithEx I feel is too specific and your use case is better replaced by regex.

Here is my use case https://github.com/Alienmario/smartdm-redux/blob/main/scripting/include/smartdm_redux.inc#L110
I feel like regex is a overkill for such a simple function.
The gains may be smaller than expected, but I like having the option.
Keep in mind strlen computation time grows with length of string.

sirdigbot · 2024-06-29T10:31:43Z

StrEndsWithEx I feel is too specific and your use case is better replaced by regex.

Here is my use case https://github.com/Alienmario/smartdm-redux/blob/main/scripting/include/smartdm_redux.inc#L110 I feel like regex is a overkill for such a simple function. The gains may be smaller than expected, but I like having the option. Keep in mind strlen computation time grows with length of string.

Yeah so this seems to just be avoiding needing to call strlen(filepath) each time you check a different file extension. Which is technically an optimisation but probably not enough of one to be built into sourcemod directly. Especially because you're still calling strlen(extension), where the slowest component of that is going to be the SP -> C++ -> SP process anyway.

While strlen is O(n), 5 redundant calls to it on even a long filepath are probably insignificant unless it's happening pretty frequently.

sirdigbot · 2024-06-29T11:51:52Z

Anyway, I've started a new PR where further comments should be added instead.

Added some new stocks

c5d22e6

Added the following stocks: - IsStringNumber - IsStringFloat - StringToLowerCase - StringToUpperCase - IndexOfChar - LastIndexOfChar Tested: Test plugin source link: https://pastebin.com/QWXUy0ta Test results: https://i.imgur.com/NKhvsl4.png

dvander reviewed Apr 7, 2020

View reviewed changes

Removed unnecessary temporary variables.

f817b9e

Removed unnecessary temporary variables.

KyleSanderson requested changes Apr 8, 2020

View reviewed changes

plugins/include/string.inc Show resolved Hide resolved

plugins/include/string.inc Show resolved Hide resolved

milutinke commented Apr 10, 2020

View reviewed changes

Update string.inc

224de89

Changed IsStringNumber and IsStringFloat to wrap StringToIntEx and StringToFloatEx. Changed IndexOfChar to wrap FindCharInString. Added CopyC

Headline added the Feature Request user requested feature label May 8, 2020

string: normalize strings for SP.

45ce1d6

If the string has a floating point value or not, we can still store it in a float. No need to check the additional stocks, they're built-in. CopyC already exists, that's simply strcopy.

KyleSanderson closed this May 20, 2021

KyleSanderson reopened this May 20, 2021

KyleSanderson approved these changes May 20, 2021

View reviewed changes

KyleSanderson requested a review from dvander March 30, 2023 04:21

Merge branch 'master' into milutinke/master

e94e682

milutinke closed this Jun 28, 2024

Added some new string manipulation stocks #1224

Added some new string manipulation stocks #1224

Conversation

milutinke commented Apr 7, 2020 • edited Loading

dvander Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KyleSanderson left a comment

Choose a reason for hiding this comment

milutinke commented Apr 10, 2020

peace-maker commented Apr 10, 2020

milutinke left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvander commented Apr 10, 2020 • edited Loading

milutinke commented Apr 10, 2020 • edited Loading

KyleSanderson commented Aug 16, 2020

KyleSanderson commented Oct 3, 2020

milutinke commented May 21, 2021 • edited Loading

sirdigbot commented Dec 26, 2021

Alienmario commented Jan 15, 2022

Alienmario commented Jan 30, 2023

peace-maker commented Jul 3, 2023

milutinke commented Jul 3, 2023

sirdigbot commented Jul 5, 2023 • edited Loading

Alienmario commented Jun 22, 2024

sirdigbot commented Jun 28, 2024

milutinke commented Jun 28, 2024

Alienmario commented Jun 28, 2024

sirdigbot commented Jun 29, 2024 • edited Loading

Alienmario commented Jun 29, 2024 • edited Loading

sirdigbot commented Jun 29, 2024

sirdigbot commented Jun 29, 2024

milutinke commented Apr 7, 2020 •

edited

Loading

dvander Apr 7, 2020 •

edited

Loading

milutinke left a comment •

edited

Loading

dvander commented Apr 10, 2020 •

edited

Loading

milutinke commented Apr 10, 2020 •

edited

Loading

milutinke commented May 21, 2021 •

edited

Loading

sirdigbot commented Jul 5, 2023 •

edited

Loading

sirdigbot commented Jun 29, 2024 •

edited

Loading

Alienmario commented Jun 29, 2024 •

edited

Loading