chore(string): Implement removeExtension() function for string classes and add find/reverseFind to UnicodeString

[Mauller]

Merge by rebase

This PR has a small chore to remove the unused static removeExtensions() function found in the gamestate.cpp file.
It also implements some missing functions within UnicodeString for find and reverseFind to give it parity with asciiString

Overall it implements the getExtension() and removeExtension() functions for AsciiString and UnicodeStrings within TheFileSystem.

[greptile-apps]

Greptile Summary

This PR removes the stale, locally-scoped removeExtension helper from both GameState.cpp copies, replaces it with proper FileSystem::removeExtension static methods for AsciiString and UnicodeString, and introduces a PathUtil.h utility header with inline getExtension overloads. It also adds find/reverseFind to UnicodeString for parity with AsciiString, and contributes maxPtr/minPtr pointer utilities to BaseType.h to safely handle the nullptr-vs-valid-pointer separator comparison that was flagged in the previous review round.

Confidence Score: 5/5

This PR is safe to merge — all previously flagged bugs have been addressed and no new issues were found.

The three prior P1 findings (sizeof truncation bug, null-pointer relational UB, ODR violation) are all resolved: pointer arithmetic is used correctly, maxPtr handles the nullptr case safely, and getExtension overloads are marked inline. No new logic, security, or correctness issues were identified.

No files require special attention.

Important Files Changed

Filename	Overview
Core/Libraries/Include/Lib/PathUtil.h	New utility header with inline getExtension overloads for char* and wchar_t*; functions are inline (ODR-safe), use maxPtr for safe null-safe separator comparison, and correctly handle paths with dots in directory components.
Core/Libraries/Include/Lib/BaseType.h	Adds maxPtr/minPtr pointer templates with nullptr-safe semantics; used to replace unsafe relational comparison between a null and a non-null pointer that was flagged in the prior review.
Core/GameEngine/Source/Common/System/FileSystem.cpp	Implements FileSystem::removeExtension for both string types using correct pointer-difference arithmetic (ext - path.str()) rather than the sizeof bug present in the earlier draft.
Core/GameEngine/Include/Common/FileSystem.h	Adds static bool removeExtension declarations for AsciiString and UnicodeString overloads; change log comment uses a 2026 date consistent with the team's rule.
Core/GameEngine/Include/Common/UnicodeString.h	Adds find/reverseFind inline methods wrapping wcschr/wcsrchr, giving UnicodeString parity with AsciiString; implementation is straightforward and correct.
Generals/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp	Removes the static removeExtension helper that was unused dead code; no functional changes to live code paths.
GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp	Same dead-code removal as the Generals variant; no functional changes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["FileSystem::removeExtension(path)"] --> B["getExtension(path.str())"]
    B --> C{"strrchr for '.'"}
    C -- "no dot found" --> D["return nullptr"]
    C -- "dot found" --> E["maxPtr(strrchr '/' , strrchr '\\')"]
    E --> F{"lastSeparator &&\nlastDot < lastSeparator?"}
    F -- "yes (dot in dir name)" --> D
    F -- "no (dot in filename)" --> G["return pointer to dot"]
    G --> H["path.truncateTo(ext - path.str())"]
    H --> I["return true"]
    D --> J["return false"]

Loading

_{Reviews (19): Last reviewed commit: "chore(filesystem): Implement getExtensio..." | Re-trigger Greptile}

[Mauller]

will fix this tomorrow

[Mauller]

Fixed before bed in the end, should be okay now and working as expected.

[xezon]

I do not like the implementation.

[Mauller]

updated based on feedback.

[Mauller]

Updated the implementation to take the dots position into account by checking if it appears before path seperators.

[Mauller]

Appeasing the AI overMunkee

[Mauller]

Helps when i do it properly.

[Mauller]

This should be good now, speeling mistake corrected and pull number added to commits.

[Mauller]

Updated to move the functionality into the file system

[Mauller]

Just a rebase with Main since BaseType is updated on main

[Mauller]

Edited based on majority of recent feedback, just waiting on discussion on two points.

[Mauller]

Updated based on feedback and implemented pointer comparison template functions.

[xezon]

Looks correct.

Maybe there can be a follow up in the future where former extension searches are replaced with the new functions.

[xezon]

Greptile says:

Comment:
"Must be C compliant" comment contradicts file contents

The comment states the file must be C-compliant, but the file uses several C++ features: function overloading (two getExtension overloads), nullptr (a C++11 keyword), and #include "BaseType.h" which is a C++ header. None of these are valid in standard C. The comment should be updated or removed to avoid misleading maintainers.

[Mauller]

Greptile says:

Comment:
"Must be C compliant" comment contradicts file contents

The comment states the file must be C-compliant, but the file uses several C++ features: function overloading (two getExtension overloads), nullptr (a C++11 keyword), and #include "BaseType.h" which is a C++ header. None of these are valid in standard C. The comment should be updated or removed to avoid misleading maintainers.

Ah missed that when copying the header.
Fixed now.

[xezon]

Greptile has another comment

Comment:
Unconstrained template parameter for pointer-only helpers

maxPtr/minPtr are documented as "should only ever be used with pointers" but the template accepts any type. Instantiating with an arithmetic or class type compiles but produces meaningless results (e.g. comparing against nullptr always falls through to an arithmetic > comparison). A static_assert would catch misuse at the point of instantiation.

template <typename PTR>
inline PTR maxPtr(PTR x, PTR y) noexcept
{
    static_assert(std::is_pointer<PTR>::value, "maxPtr is for pointer types only");
    // ...
}

Same for minPtr.

[Mauller]

Greptile has another comment

Comment: Unconstrained template parameter for pointer-only helpers

maxPtr/minPtr are documented as "should only ever be used with pointers" but the template accepts any type. Instantiating with an arithmetic or class type compiles but produces meaningless results (e.g. comparing against nullptr always falls through to an arithmetic > comparison). A static_assert would catch misuse at the point of instantiation.

template <typename PTR>
inline PTR maxPtr(PTR x, PTR y) noexcept
{
    static_assert(std::is_pointer<PTR>::value, "maxPtr is for pointer types only");
    // ...
}

Same for minPtr.

Had to double check if that would work with VC6 since is_pointer() is C++11. But static_assert gets defined out for VC6

[Mauller]

updated with static asserts.

[xezon]

The commits are titled as "feat" which implies that a new feature was added. But isn't this more a chore than feature? Because user facing nothing changes.

[xezon]

This comment can now be removed because the static assert already documents and enforces it sufficiently.

[Mauller]

fixed

[Mauller]

The commits are titled as "feat" which implies that a new feature was added. But isn't this more a chore than feature? Because user facing nothing changes.

I didn't think it needed to be considered user facing in context of the game itself. will change it to chore.

[Mauller]

cleaned up the commit titles a touch as well

[xezon]

I didn't think it needed to be considered user facing in context of the game itself. will change it to chore.

I am not sure either. But typically we would expect a feature as something the user can see. Adding a few new functions that do not really change the game behavior are probably not recognized as a new feature.