refactor(pathutil): Add appendPathSeparator for character buffers by bobtista · Pull Request #3444 · TheSuperHackers/GeneralsGameCode

bobtista · 2026-10-08T16:04:45Z

Follow up to #3362. Merge that first.

Adds appendPathSeparator to PathUtil.h. It appends the native separator when the path does not already end with one. Replaces the hand-written trailing separator checks in ImagePacker, W3DView, Autorun, WWLib, WW3D2 and WorldBuilder.

TODO: Replicate in Generals.

coderabbitai · 2026-10-08T16:05:10Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Last reviewed commit: 382efc8a3e5b6cffb1520a0484917732710db959
Latest commit, not reviewed: e9df58f

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review.

Walkthrough

The pull request extends shared path utilities and updates engine, library, and tool code to use them. The changes replace backslash-specific parsing in path operations and filename extraction. They also remove two file-transfer helpers and one library filename helper.

Changes

Shared path handling

Layer / File(s)	Summary
Path utility and file-transfer APIs `Core/Libraries/Include/Lib/PathUtil.h`, `Core/GameEngine/Include/GameNetwork/FileTransfer.h`, `Core/GameEngine/Source/GameNetwork/FileTransfer.cpp`	`PathUtil.h` adds wide-character and mutable-pointer overloads, a first-separator lookup, and `appendPathSeparator`. File-transfer code uses shared path and extension helpers and removes `GetFileFromPath` and `GetExtensionFromFile`.
Engine path operations and map names `Core/GameEngine/Source/Common/`, `Core/GameEngine/Source/GameClient/`, `Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp`, `Core/GameEngine/Source/GameNetwork/`, `Core/GameEngineDevice/Source/`	Engine code uses shared helpers to locate separators, extract filenames, append separators, and parse archive paths. Map-name fallbacks, path matching, INI loading, and preview-image naming also use the helpers.
Library assets, audio, and diagnostics `Core/Libraries/Source/WWVegas/`, `Core/Libraries/Source/debug/`	Library code uses shared helpers for asset and sound filenames, download directory scanning, file-factory paths, and debug output. The inline `Get_Filename_From_Path` helper is removed.
Core tools and launcher paths `Core/Tools/Autorun/`, `Core/Tools/ImagePacker/Source/`, `Core/Tools/Launcher/`, `Core/Tools/W3DView/`, `Core/Tools/WW3D/max2w3d/w3dutil.cpp`	Tool code uses shared helpers for filename extraction, executable-directory lookup, separator checks, and path delimiting.
Generals game-engine paths `Generals/Code/GameEngine/Source/`, `Generals/Code/Libraries/Source/WWVegas/WW3D2/part_ldr.cpp`, `GeneralsMD/Code/GameEngine/Source/`, `GeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/part_ldr.cpp`	Both game variants use shared helpers for replay names, map names, save-game labels, and map-directory paths.
Generals and GeneralsMD tool paths `Generals/Code/Tools/`, `GeneralsMD/Code/Tools/`	WorldBuilder and GUI tools use shared helpers for path delimiters, executable paths, and filename extraction. GeneralsMD wdump also uses the filename helper.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Refactor

Suggested reviewers: xezon

Merge Risk: Low · previous review 382ef

The path-handling refactor appears behavior-preserving for archive loading. One minor open concern remains in the W3DView Delimit_Path helper: it can mishandle an empty path. It should be addressed or explicitly accepted.

Pre-merge checks | 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check		The title clearly identifies the main change: adding appendPathSeparator to PathUtil for character buffers.
Description check		The description accurately describes the new helper, its behavior, affected areas, dependency on `#3362`, and the remaining Generals TODO.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: `acbbee24-a5bc-4724-a356-849fb31fd890`

📥 Commits

Reviewing files that changed from the base of the PR and between b64c8c654fa14ee1e02155ab73cb67653fb3b149 and 382efc8a3e5b6cffb1520a0484917732710db959.

📒 Files selected for processing (17)

`Core/GameEngine/Source/Common/CommandLine.cpp`

`Core/GameEngine/Source/Common/INI/INI.cpp`

`Core/GameEngine/Source/Common/ReplaySimulation.cpp`

`Core/GameEngine/Source/GameClient/GUI/GameWindowManagerScript.cpp`

`Core/GameEngineDevice/Source/StdDevice/Common/StdBIGFileSystem.cpp`

`Core/GameEngineDevice/Source/Win32Device/Common/Win32BIGFileSystem.cpp`

`Core/Libraries/Include/Lib/PathUtil.h`

`Core/Libraries/Source/WWVegas/WWAudio/Utils.h`

`Core/Libraries/Source/WWVegas/WWDownload/FTP.cpp`

`Core/Tools/Launcher/findpatch.cpp`

`Core/Tools/Launcher/patch.cpp`

`Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp`

`Generals/Code/Tools/WorldBuilder/src/TerrainMaterial.cpp`

`Generals/Code/Tools/WorldBuilder/src/TerrainModal.cpp`

`GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp`

`GeneralsMD/Code/Tools/WorldBuilder/src/TerrainMaterial.cpp`

`GeneralsMD/Code/Tools/WorldBuilder/src/TerrainModal.cpp`

💤 Files with no reviewable changes (1)

Core/Libraries/Source/WWVegas/WWAudio/Utils.h

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

coderabbitai

Actionable comments posted: 2

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 1a9d0ce7-d8fc-4873-a0c0-c8cd27bd0843

📥 Commits

Reviewing files that changed from the base of the PR and between 9176130 and 6700aad51d9f5e930cf4a9f306171a02352d2437.

📒 Files selected for processing (80)

Core/GameEngine/Include/GameNetwork/FileTransfer.h
Core/GameEngine/Source/Common/Audio/AudioEventRTS.cpp
Core/GameEngine/Source/Common/CRCDebug.cpp
Core/GameEngine/Source/Common/INI/INIMapCache.cpp
Core/GameEngine/Source/Common/MiniLog.cpp
Core/GameEngine/Source/Common/System/Debug.cpp
Core/GameEngine/Source/Common/System/FileSystem.cpp
Core/GameEngine/Source/Common/System/GameMemoryInit.cpp
Core/GameEngine/Source/Common/WorkingDirectory.cpp
Core/GameEngine/Source/GameClient/MapUtil.cpp
Core/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp
Core/GameEngine/Source/GameNetwork/FileTransfer.cpp
Core/GameEngine/Source/GameNetwork/GameInfo.cpp
Core/GameEngine/Source/GameNetwork/GameSpy/LobbyUtils.cpp
Core/GameEngineDevice/Source/MilesAudioDevice/MilesAudioManager.cpp
Core/GameEngineDevice/Source/W3DDevice/GameClient/Drawable/Draw/W3DModelDraw.cpp
Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplay.cpp
Core/Libraries/Include/Lib/PathUtil.h
Core/Libraries/Source/WWVegas/WW3D2/agg_def.cpp
Core/Libraries/Source/WWVegas/WW3D2/ringobj.cpp
Core/Libraries/Source/WWVegas/WW3D2/sphereobj.cpp
Core/Libraries/Source/WWVegas/WW3D2/w3d_dep.cpp
Core/Libraries/Source/WWVegas/WWAudio/AudibleSound.cpp
Core/Libraries/Source/WWVegas/WWAudio/Utils.h
Core/Libraries/Source/WWVegas/WWDownload/FTP.cpp
Core/Libraries/Source/WWVegas/WWLib/WWCommon.h
Core/Libraries/Source/WWVegas/WWLib/ffactory.cpp
Core/Libraries/Source/debug/debug_debug.cpp
Core/Libraries/Source/debug/debug_io_flat.cpp
Core/Libraries/Source/debug/debug_stack.cpp
Core/Tools/Autorun/Utils.cpp
Core/Tools/Autorun/Wnd_file.cpp
Core/Tools/ImagePacker/Source/ImagePacker.cpp
Core/Tools/ImagePacker/Source/WindowProcedures/DirectorySelect.cpp
Core/Tools/Launcher/DatGen/DatGen.cpp
Core/Tools/Launcher/findpatch.cpp
Core/Tools/Launcher/main.cpp
Core/Tools/Launcher/patch.cpp
Core/Tools/W3DView/GraphicView.cpp
Core/Tools/W3DView/MainFrm.cpp
Core/Tools/W3DView/SaveSettingsDialog.cpp
Core/Tools/W3DView/Utils.cpp
Core/Tools/W3DView/Utils.h
Core/Tools/W3DView/W3DViewDoc.cpp
Core/Tools/WW3D/max2w3d/w3dutil.cpp
Generals/Code/GameEngine/Source/Common/Recorder.cpp
Generals/Code/GameEngine/Source/Common/StatsCollector.cpp
Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/DownloadMenu.cpp
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/GameInfoWindow.cpp
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/LanGameOptionsMenu.cpp
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/PopupSaveLoad.cpp
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/ReplayMenu.cpp
Generals/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/WOLGameSetupMenu.cpp
Generals/Code/GameEngine/Source/GameLogic/Map/TerrainLogic.cpp
Generals/Code/Libraries/Source/WWVegas/WW3D2/part_ldr.cpp
Generals/Code/Tools/GUIEdit/Source/GUIEdit.cpp
Generals/Code/Tools/WorldBuilder/src/BuildList.cpp
Generals/Code/Tools/WorldBuilder/src/SaveMap.cpp
Generals/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp
Generals/Code/Tools/WorldBuilder/src/WorldBuilderDoc.cpp
GeneralsMD/Code/GameEngine/Source/Common/Recorder.cpp
GeneralsMD/Code/GameEngine/Source/Common/StatsCollector.cpp
GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/DownloadMenu.cpp
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/GameInfoWindow.cpp
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/LanGameOptionsMenu.cpp
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/PopupSaveLoad.cpp
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/ReplayMenu.cpp
GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/WOLGameSetupMenu.cpp
GeneralsMD/Code/Libraries/Source/WWVegas/WW3D2/part_ldr.cpp
GeneralsMD/Code/Tools/GUIEdit/Source/GUIEdit.cpp
GeneralsMD/Code/Tools/WorldBuilder/src/BuildList.cpp
GeneralsMD/Code/Tools/WorldBuilder/src/MeshMoldOptions.cpp
GeneralsMD/Code/Tools/WorldBuilder/src/SaveMap.cpp
GeneralsMD/Code/Tools/WorldBuilder/src/ScriptDialog.cpp
GeneralsMD/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp
GeneralsMD/Code/Tools/WorldBuilder/src/WorldBuilderDoc.cpp
GeneralsMD/Code/Tools/wdump/wdump.cpp

💤 Files with no reviewable changes (2)

Core/GameEngine/Include/GameNetwork/FileTransfer.h
Core/GameEngine/Source/Common/System/FileSystem.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

greptile-apps · 2026-11-06T14:32:30Z

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new actionable issues were found.

Summary

Adds ensureTrailingSeparatorInPath for character buffers and replaces repeated trailing-separator checks.

Path-building tools now use one shared trailing-separator rule.
W3DView checks empty paths before adding a trailing separator.

_{Reviews (2) · Last reviewed commit: "refactor(generals): Rename appendPathSep..." · Reviewed by Greptile}

xezon

Are we certain this covers all call sites?

xezon · 2026-11-07T10:35:43Z

 	return lastSeparator ? lastSeparator + 1 : path;
 }

+inline bool appendPathSeparator(char* path, size_t size)


Can you make this a template functions for both char types?

Done, it's a template for both char types now.

xezon · 2026-11-07T10:36:56Z

 	return lastSeparator ? lastSeparator + 1 : path;
 }

+inline bool appendPathSeparator(char* path, size_t size)


The placement in this file looks not optimal, because it unfavourably squeezed between getFileName and getExtension.

Moved it to the end of the file after getExtension.

xezon · 2026-11-07T10:39:55Z

 __inline void Delimit_Path (CString &path)
 {
-	if (!isPathSeparator (path[::lstrlen (path) - 1])) {
+	if (::lstrlen (path) > 0 && !isPathSeparator (path[::lstrlen (path) - 1])) {


Move 2x lstrlen out condition and reuse result.

Done, lstrlen is called once now.

xezon · 2026-11-07T10:48:55Z

 	return lastSeparator ? lastSeparator + 1 : path;
 }

+inline bool appendPathSeparator(char* path, size_t size)


Maybe we should return the new length as well? It is used at a few call sites.

struct AppendPathSeparatorResult { bool ok; size_t length; };

Done, it returns EnsureTrailingSeparatorInPathResult with ok and length now. ffactory uses the length instead of calling strlen again.

xezon · 2026-11-07T10:51:49Z

 	return lastSeparator ? lastSeparator + 1 : path;
 }

+inline bool appendPathSeparator(char* path, size_t size)


appendPathSeparator implies that a path separator is appended. But that is not always true: when strlen is 0 or it already ends with a path sep, then no path sep is appended but the function returns true. Maybe it needs a different name? For example ensureTrailingSeparatorInPath.

Maybe also add a description comment.

Renamed it to ensureTrailingSeparatorInPath and added a description comment.

bobtista · 2026-11-07T17:21:35Z

No, not all of them. Autorun/Utils.cpp:397 and :407 (Path_Add_Back_Slash) and W3DView/Utils.h:65 and :72 (Delimit_Path) still do it by hand, because they take a path with no buffer size. The Babylon and DatGen strcat calls have no existing-separator check, so I left them. If you want the unsized ones covered too, I'll add overloads for them.

…atorInPath and return the path length

…atorInPath

coderabbitai Bot reviewed Oct 8, 2026

View reviewed changes

Comment thread Core/Libraries/Include/Lib/PathUtil.h Resolved

Comment thread Core/Tools/W3DView/Utils.h Resolved

bobtista force-pushed the bobtista/refactor/append-path-separator branch 5 times, most recently from 382efc8 to 63de2c4 Compare October 11, 2026 19:44

bobtista force-pushed the bobtista/refactor/append-path-separator branch 5 times, most recently from 962122f to 5ec1b8c Compare October 19, 2026 00:16

bobtista mentioned this pull request Oct 19, 2026

bugfix(imagepacker): Skip folders whose path does not fit in the add folder buffer #3558

Draft

1 of 2 tasks

bobtista force-pushed the bobtista/refactor/append-path-separator branch from 5ec1b8c to 641b280 Compare October 21, 2026 04:35

bobtista force-pushed the bobtista/refactor/append-path-separator branch from 641b280 to 9abcce4 Compare October 28, 2026 18:08

bobtista added 3 commits November 5, 2026 11:51

refactor(pathutil): Add appendPathSeparator for character buffers

7443610

refactor(generals): Add appendPathSeparator for character buffers

4604a67

fix(w3dview): Guard Delimit_Path against an empty CString

7cdd6b3

bobtista force-pushed the bobtista/refactor/append-path-separator branch from 9abcce4 to 7cdd6b3 Compare November 5, 2026 16:52

bobtista marked this pull request as ready for review November 6, 2026 14:29

xezon reviewed Nov 7, 2026

View reviewed changes

xezon added Minor Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing labels Nov 7, 2026

bobtista added 2 commits November 7, 2026 12:23

refactor(pathutil): Rename appendPathSeparator to ensureTrailingSepar…

302a3ae

…atorInPath and return the path length

refactor(generals): Rename appendPathSeparator to ensureTrailingSepar…

e9df58f

…atorInPath

Conversation

bobtista commented Oct 8, 2026

Uh oh!

coderabbitai Bot commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

greptile-apps Bot commented Nov 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 5/5

Summary

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!

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!

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!

bobtista commented Nov 7, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Oct 8, 2026 •

edited

Loading

greptile-apps Bot commented Nov 6, 2026 •

edited

Loading