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

Add support for other character types in filenames (fixes #6) #7

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MHebes
Copy link

@MHebes MHebes commented Jan 17, 2023

Allows unicode filenames on windows, which uses UTF-16.

@MHebes
Copy link
Author

MHebes commented Feb 13, 2023

Hi @sreiter, incorporated your requested changes from #6 (comment)

Copy link
Owner

@sreiter sreiter left a comment

Choose a reason for hiding this comment

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

Thanks so much for adjusting the template parameter name!
I just saw that you removed a method from the public interface. I think that's a bit unfortunate, since some users may already rely on that method. It would be great if you would consider my comment and reintroduce it - ideally togehter with an additional wstring overload.
Thank's a lot!

Comment on lines -284 to -287
bool read_file (const std::string& filename)
{
return read_file (filename.c_str());
}
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, I didn't see this last time. But it seems that you removed this method? I think it is in the public interface, so some users may rely on it. Unless there's a good reason to remove it, I would thus rather keep it. And if we have an std::string overload, I think it would even make sense to add a second overload with const std::wstring&, as you did in the constructor...

@MHebes
Copy link
Author

MHebes commented Nov 29, 2023

Hi @sreiter, sorry for the delay in getting this PR updated.

Since it was originally made, I realized that these two macros are preventing this from working perfectly:

  #define STL_READER_THROW(msg) {std::stringstream ss; ss << msg; throw(std::runtime_error(ss.str()));}
  #define STL_READER_COND_THROW(cond, msg)  if(cond){std::stringstream ss; ss << msg; throw(std::runtime_error(ss.str()));}

These are problematic with wchar_t e.g.

          STL_READER_THROW("ERROR while reading from " << /* const wchar_t* */ filename <<
            ": vertex not specified correctly in line " << lineCount);

where if filename was a const wchar_t*, it would print as a pointer value instead of a string in <C++20.

In C++20, they actually deleted operator<<(basic_ostream<char, Traits>&, wchar_t) so this would actually become a compiler error with this PR when TChar = wchar_t.

Options:

  1. Rely on a macro to manually switch types around (UNICODE, _UNICODE, or STL_READER_USE_WCHAR) or use something like WideCharToMultiByte) that converts to UTF8 when needed
  2. Use C++17's filesystem::path, which automatically accepts either character type and provides a operator<< for stringstream, or
  3. Drop this PR, I'm perfectly fine with just maintaining a fork.

Please let me know how you'd like to proceed!

@sreiter
Copy link
Owner

sreiter commented Dec 2, 2023

Hi @MHebes, great to hear from you. Maybe a small overloaded function which returns the utf8 string for wchar__t and the plain string for the classic char would be simple enough. I will try to look into it to reproduce the issue.
Thanks 😃

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

Successfully merging this pull request may close these issues.

2 participants