Skip to content

chore(pathutil): Add path separator helpers - #3105

Merged
xezon merged 7 commits into
TheSuperHackers:mainfrom
bobtista:bobtista/refactor/path-separator-helpers
Sep 26, 2026
Merged

xezon merged 7 commits into
TheSuperHackers:mainfrom
bobtista:bobtista/refactor/path-separator-helpers

Conversation

@bobtista

@bobtista bobtista commented Aug 11, 2026 •

Copy link
Copy Markdown

Extract the separator search from getExtension into reusable helpers in PathUtil.h, and add FileSystem::appendPathSeparator for the map and archive fixes in #3107 and #3141.

Keep native path separators separate from game-data separators, since game paths can contain backslashes on any platform. Existing getExtension and isAbsolutePath behavior stays unchanged.

All changes are in Core.

@bobtista
bobtista force-pushed the bobtista/refactor/path-separator-helpers branch from 308e9ec to 54957e2 Compare September 14, 2026 21:06
@bobtista bobtista changed the title refactor(lib): Add path separator helpers to PathUtil refactor(lib): Add shared path separator helpers Sep 14, 2026
@bobtista bobtista changed the title refactor(lib): Add shared path separator helpers refactor(lib): Add path separator helpers Sep 14, 2026
bobtista added a commit to bobtista/GeneralsGameCode that referenced this pull request Sep 15, 2026
@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: ffe22636-f664-417c-ac58-ce3a4b9a40b8

📥 Commits

Reviewing files that changed from the base of the PR and between 1010254 and d7a099d.

📒 Files selected for processing (4)
  • Core/GameEngine/Include/Common/AsciiString.h
  • Core/GameEngine/Include/Common/UnicodeString.h
  • Core/GameEngine/Source/Common/System/FileSystem.cpp
  • Core/Libraries/Include/Lib/PathUtil.h

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

Path utilities distinguish host-platform separators from separators recognized across platforms, and add helpers for separator lookup and filename extraction. AsciiString and UnicodeString add character accessors. FileSystem adds an operation to append the native separator when needed.

Changes

Path Utilities

Layer / File(s) Summary
Separator checks and path helpers
Core/Libraries/Include/Lib/PathUtil.h
PathUtil distinguishes host-platform separators from separators recognized on all platforms. It adds native-separator lookup, last-separator helpers, and getFileName. Absolute-path and extension checks use the updated separator helpers.
String character accessors
Core/GameEngine/Include/Common/AsciiString.h, Core/GameEngine/Include/Common/UnicodeString.h
AsciiString and UnicodeString add front() and back() accessors.
Append native separator
Core/GameEngine/Include/Common/FileSystem.h, Core/GameEngine/Source/Common/System/FileSystem.cpp
FileSystem declares and implements appendPathSeparator. It appends the native separator to a nonempty path unless either slash character is already at the end.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Suggested reviewers: xezon

Merge Risk: ⚪ Minimal · up to d7a09

The path utilities preserve the existing absolute-path behavior, and the new string and separator helpers follow their stated contracts. No actionable merge-blocking risk was found.

Security Architecture Review

Security architecture risk: 🔵 Low · up to d7a09

The path helpers distinguish game-data separators from separators used by the host filesystem. The new filesystem helper guards its use of the string accessor, and no exploitable path is established. How future callers will use these public helpers remains uncertain.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed separator semantics could affect consumers of shared Core path helpers, but the available relationships do not establish an attacker-controlled entrypoint or sensitive sink for a changed call path.

Trust Boundaries and Controls

  • observed — The introduced FileSystem caller guards the accessor’s nonempty precondition. Separator classification differs by purpose: both slash forms for game-data paths, host separators for filesystem-path checks.

Hardening Proposals

  • proposed — When adopting appendPathSeparator for filesystem joins, verify how callers handle a trailing backslash on non-Windows hosts: the helper treats it as a game-data separator although the host-specific predicate does not.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the main change: adding path separator helpers. It is concise and specific.
Description check ✅ Passed The description accurately covers the reusable PathUtil helpers, FileSystem::appendPathSeparator, separator behavior, and related fixes.

Comment @coderabbitai help to get the list of available commands.

@bobtista
bobtista marked this pull request as ready for review September 24, 2026 21:40
@greptile-apps

greptile-apps Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Adds string utility methods and path separator helpers.

The PR appears safe to merge; no actionable new issue was identified.

Summary

This PR extracts reusable path-separator helpers and adds string accessors and FileSystem::appendPathSeparator.

  • Native filesystem separators remain distinct from separators accepted in game-data paths.
  • The changes since the previous review preserve the existing append behavior.

Reviews (2) · Last reviewed commit: "refactor(strings): Add read-only front a..."

Comment thread Core/GameEngine/Source/Common/System/FileSystem.cpp Outdated
Comment thread Core/Libraries/Include/Lib/PathUtil.h Outdated

@xezon xezon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Makes sense.


// Requires a nonempty string.
char front() const;
char back() const;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I suggest also add an explanation why getCharAt, front, back do not return char&. Because this is a copy-on-write string and it would not work unlike std::string which is always a unique copy.

#include "BaseType.h"
#include <string.h>

inline char getNativePathSeparator()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actually you can move it back to where it was before. I see now you had them sorted by is and get which made sense.

@xezon xezon changed the title refactor(lib): Add path separator helpers chore(pathutil): Add path separator helpers Sep 25, 2026
@xezon
xezon merged commit 3f136a8 into TheSuperHackers:main Sep 26, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Platform Work towards platform support, such as Linux, MacOS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants