Skip to content

Fix AbsolutePath returning non identical strings on semantically identical paths - #17355

Open
DeltachangeOG wants to merge 3 commits into
luanti-org:masterfrom
DeltachangeOG:fix-unit-test-and-errors
Open

Fix AbsolutePath returning non identical strings on semantically identical paths#17355
DeltachangeOG wants to merge 3 commits into
luanti-org:masterfrom
DeltachangeOG:fix-unit-test-and-errors

Conversation

@DeltachangeOG

Copy link
Copy Markdown
Contributor

Add compact, short information about your PR for easier understanding:

AbsolutePath should return identical strings for semantically identical strings but previously could return: C:/Test/Path and C:/Test/Path/ depending on what the function was called on. This lead to failing tests because other functions expected identical outputs. The invalid string position would wrongfully assume that one path had a delimiter and then move to the other side of the delimiter to attempt comparison but since they were the same path it was an out of bounds index.

Part of #17354

To do

This PR is Ready for Review.

I added unit tests that ensure AbsolutePath returns as expected. I also tested master branch for non ASCII character handling(it doesn't) to ensure my fix didn't break previous functionality

Comment thread src/filesys.cpp Outdated
@Zughy Zughy added Bugfix 🐛 PRs that fix a bug @ Engine Core What happens inside the very engine labels Jul 18, 2026
Comment thread .gitignore Outdated
@DeltachangeOG
DeltachangeOG force-pushed the fix-unit-test-and-errors branch from 5f08cfb to ac9ef23 Compare July 18, 2026 11:06
@DeltachangeOG
DeltachangeOG force-pushed the fix-unit-test-and-errors branch from ac9ef23 to dfb011a Compare July 18, 2026 11:21
@DeltachangeOG
DeltachangeOG requested a review from ZenonSeth July 18, 2026 11:22
Comment thread src/filesys.cpp Outdated
Comment thread src/filesys.cpp Outdated
Co-authored-by: DS <7613443+Desour@users.noreply.github.com>

@Desour Desour left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

UASSERTCMP(auto, !=, fs::AbsolutePath(dir_path2), ""); // now it does
const auto absolute_dir_path = fs::AbsolutePath(dir_path2);
UASSERTCMP(auto, !=, absolute_dir_path, "");// now it does
const std::filesystem::path absolute_path(absolute_dir_path);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also needs native_format.

You might want to add a fs::getRootPathOf(path).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bugfix 🐛 PRs that fix a bug @ Engine Core What happens inside the very engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants