[GEN][ZH] Simplify repeated calls to removeLastChar with new truncate and trim methods #1257

slurmlord · 2025-07-10T17:38:31Z

Currently the only way to remove characters from the end of a string (AsciiString, UnicodeString, DisplayString) is by calling removeLastChar. If there are multiple characters that needs to be removed, removeLastChar must be called in a loop.
Each call to removeLastChar results in a call to strlen or wcslen, which is quite wasteful when done in a loop.

This change introduces 4 new methods for string classes to alleviate this; truncateBy, truncateTo, trimEnd() and trimEnd(char).
truncateBy will remove a given number of characters from the end of a string.
truncateTo will remove characters from the end of the string until it is of a given length.
Each of these will result in a single call to strlen or wcslen, thus saving an extra call per character beyond the first one.
trimEnd() exposes the end-part from trim as a separate function, where all consecutive white space characters are removed from the string.
trimEnd(char) will remove all consecutive characters of a given value from the end of the string.

The truncation functionality is extracted from #1119 as a separate PR to keep it cleaner.

Replace repeated calls to removeLastChar in various places in the code base with single calls to truncateBy, truncateTo or trimEnd.

xezon

Looks good in principle.

Going forward we can also optimize the string classes to store their length.

xezon · 2025-07-10T19:40:36Z

GeneralsMD/Code/Tools/WorldBuilder/src/WorldBuilderDoc.cpp

+	{
+		++numNonSep;
+	}
+	s.truncateBy(numNonSep);


This code is very difficult to read :(

Can this be written shorter or be a named function?

Yeah, seems like it can be written a lot clearer.
Basically a findLast('\') and a truncate to the diff from the start of the string like in other places in this PR. Must have been a Gehirnfurz.

xezon · 2025-07-10T19:47:15Z

GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/WOLLoginMenu.cpp

+
+									if (index < len - 1)
+									{
+										uniLine.truncateTo(index + 1);


This looks like its trimming trailing characters? Perhaps expose that as a function in AsciiString as well? See AsciiString::trim

~~Good suggestion - I will add something like trimEnd(const char) and update the corresponding places to use that instead.~~
I Think I read too quickly. I'll add trimEnd().

xezon · 2025-07-10T19:49:57Z

GeneralsMD/Code/GameEngine/Source/Common/System/UnicodeString.cpp

+
+			if (index < len - 1)
+			{
+				truncateTo(index + 1);


Is this correct? index + 1?

I think so; index at this point contains the index of the last character that is not a space.
As an example, if the index is 2 then m_data[2] is the character we want the string to stop after. That translates to a total string length of 3, and truncateTo will then set m_data[3] to '\0'.

Is this perhaps an early warning sign of confusing naming/description of the function and its parameter?

Maybe the - + 1 stuff can be simplified by starting looping at index = len and then look at index - 1 to test the whitespace char.

GeneralsMD/Code/GameEngine/Source/Common/System/AsciiString.cpp

…of removeLastChar.

slurmlord · 2025-07-12T20:47:20Z

Core/GameEngine/Source/Common/ReplaySimulation.cpp

@@ -215,19 +215,18 @@ std::vector<AsciiString> ReplaySimulation::resolveFilenameWildcards(const std::v
 			AsciiString dir1 = TheRecorder->getReplayDir();
 			AsciiString dir2 = *filename;
 			AsciiString wildcard = *filename;
+			const char* lastSep = dir2.reverseFind('\\');


The approach here assumes that there's not a mix of separator characters in a file name.
As an example, c:\\some\\path/here would set dir2 to c:\\some\\ instead of c:\\some\\path/.

Is this a plausible possibility?

To avoid such problem you can search for both and then take the address that is larger.

Yes, this string contains userinput from commandline, so it can contain /. I think we can just use the old code here. This will get replaced anyway as soon as we have a helper function for this stuff.

xezon · 2025-07-13T10:12:05Z

Is this meant to be a Draft Review?

xezon

Looks good to me.

xezon · 2025-07-13T20:50:20Z

Core/GameEngine/Source/Common/ReplaySimulation.cpp

This file just has whitespace changes now.

xezon · 2025-07-13T20:52:39Z

Generals/Code/GameEngine/Include/Common/AsciiString.h

+	/**
+		Truncate the string to a length of maxLength characters, not including null termination,
+		by removing from the end.
+		If the string is empty or shorter than maxLength, do nothing.


Can move this line right behind above sentence without new line.

…n comments.

slurmlord · 2025-07-13T23:20:32Z

Thanks all for the time and effort taken to review and provide feedback on this - the code has improved a lot, and as always many valuable lessons learned.

Generals/Code/GameEngine/Source/Common/System/AsciiString.cpp

slurmlord added 4 commits July 4, 2025 18:24

Add truncateBy and truncateTo methods to string classes.

f53f178

Merge remote-tracking branch 'origin/main' into string-truncate

f1064ea

Update trim to use truncateTo

b82d101

Replace repeated use of removeLastChar() with truncateBy or truncateTo.

b5c039a

xezon reviewed Jul 10, 2025

View reviewed changes

xezon added Minor Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing labels Jul 10, 2025

slurmlord added 2 commits July 10, 2025 23:06

Use truncateTo instead of truncateBy for file name

a61b34a

Use known size for ascii chars.

d10b002

helmutbuhler reviewed Jul 11, 2025

View reviewed changes

GeneralsMD/Code/GameEngine/Source/Common/System/AsciiString.cpp Outdated Show resolved Hide resolved

slurmlord added 4 commits July 12, 2025 21:13

Add trimEnd methods and use Int for parameters.

f5fc609

Use trimEnd methods.

38ff97e

Backport string functions to Generals.

c472df8

Backport to Generals: Use trimEnd, truncateBy and truncateTo instead …

103fb41

…of removeLastChar.

slurmlord changed the title ~~[ZH] Add string truncation to replace repeated calls to removeLastChar~~ [GEN][ZH] Add string truncation to replace repeated calls to removeLastChar Jul 12, 2025

slurmlord commented Jul 12, 2025

View reviewed changes

slurmlord added 2 commits July 13, 2025 19:36

Merge remote-tracking branch 'origin/main' into string-truncate

d976040

Revert ReplaySimulation changes as per review discussions.

b3f335d

slurmlord marked this pull request as ready for review July 13, 2025 17:41

xezon approved these changes Jul 13, 2025

View reviewed changes

Revert ReplaySimulation to origin/main, remove superfluous newlines i…

4569dff

…n comments.

xezon changed the title ~~[GEN][ZH] Add string truncation to replace repeated calls to removeLastChar~~ [GEN][ZH] Replace repeated calls to removeLastChar with new truncation methods Jul 14, 2025

xezon changed the title ~~[GEN][ZH] Replace repeated calls to removeLastChar with new truncation methods~~ [GEN][ZH] Replace repeated calls to removeLastChar with new truncate and trim methods Jul 14, 2025

xezon changed the title ~~[GEN][ZH] Replace repeated calls to removeLastChar with new truncate and trim methods~~ [GEN][ZH] Simplify repeated calls to removeLastChar with new truncate and trim methods Jul 14, 2025

xezon merged commit d11ecb9 into TheSuperHackers:main Jul 14, 2025
17 checks passed

Caball009 reviewed Jul 14, 2025

View reviewed changes

Generals/Code/GameEngine/Source/Common/System/AsciiString.cpp Show resolved Hide resolved

[GEN][ZH] Simplify repeated calls to removeLastChar with new truncate and trim methods #1257

[GEN][ZH] Simplify repeated calls to removeLastChar with new truncate and trim methods #1257

Uh oh!

Conversation

slurmlord commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slurmlord Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xezon commented Jul 13, 2025

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slurmlord commented Jul 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

slurmlord commented Jul 10, 2025 •

edited

Loading

slurmlord Jul 10, 2025 •

edited

Loading