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

Filename-taking methods do not work with Unicode filenames on windows #6

Open
MHebes opened this issue Jan 17, 2023 · 1 comment
Open

Comments

@MHebes
Copy link

MHebes commented Jan 17, 2023

Nice library!

The ReadStlFile_ASCII, ReadStlFile_BINARY, and StlFileHasASCIIFormat functions all take const char* for the filename.

On windows this causes issues when opening filenames which contain Unicode (UTF-16) strings.

The issue is the ifstream(const char*) constructor, which on windows will only accept ascii strings.

Possible solutions/ideas:

  • Templatize the function on CharT
  • Overload the function on windows only with a version which takes a const wchar_t* or possibly const char16_t* (this is the easiest to monkey-patch in lieu of a fix)
  • (C++17 only) Take a std::filesystem::path as an argument instead of a const char*, which should do all the conversions for you and just do the right thing on all platforms. Would be backwards compatible, since const char* converts to a path implicitly.
  • Have the function take an arbitrary istream instead of requiring filenames (would support e.g. piping easier this way I guess)
MHebes pushed a commit to MHebes/stl_reader_win_unicode_patch that referenced this issue Jan 17, 2023
@sreiter
Copy link
Owner

sreiter commented Jan 28, 2023

Hi MHebes,
thanks for the the nice words and your suggestions! Your proposed approach using an additional template argument looks like the most simple one to me. Since most functions are template functions anyways, it doesn't really change much.

For some reason I went with TTypename and not TypenameT for the template type names. Would you mind changing your CharT to TChar? it would look a little more consistent then... 😅

Thanks for your contribution! Looking forward to merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants