Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the handling of path string #528

Merged
merged 8 commits into from
Jan 18, 2024
Merged

Conversation

umireon
Copy link
Member

@umireon umireon commented Jan 13, 2024

Closes #525

Given std::string s, s.c_str() will be released when s is released. If you want to use s.c_str() later, you have to hold s until you use s.c_str().

On Windows, char paths are stored as multi-byte characters which Windows defines and it is hard to manipulate them with C++ standard libraries (codecvt was deprecated in C++17) so I propose to convert multi-byte characters with Windows-specific API that is guaranteed to work with multi-byte characters on Windows.

royshil
royshil previously approved these changes Jan 13, 2024
Copy link
Collaborator

@royshil royshil left a comment

Choose a reason for hiding this comment

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

This checks out
But make sure you test on Windows with a model path that has Unicode characters in it . This caused a bunch of problems in the past

@umireon
Copy link
Member Author

umireon commented Jan 13, 2024

I will check on Windows but it uses std::wstring on Windows.

@umireon umireon dismissed royshil’s stale review January 13, 2024 05:17

Have to fix Windows path handling

@umireon umireon changed the title Fix the uninitialized pointer Improve the handling of path string Jan 13, 2024
@umireon umireon requested a review from royshil January 13, 2024 06:10
@umireon
Copy link
Member Author

umireon commented Jan 13, 2024

I have checked it on Windows having multi-byte characters on the path and noticed that the current path handling wouldn't work under some conditions so I have added a code to handle multi-byte characters properly.

Please review again, @royshil

Copy link
Collaborator

@royshil royshil left a comment

Choose a reason for hiding this comment

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

looks good!

@umireon umireon merged commit 403dc76 into main Jan 18, 2024
6 checks passed
@umireon umireon deleted the umireon.fix-uninitialized-pointer branch January 18, 2024 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

suspected use of bad pointer in log message
2 participants