Bump to C++17 for std::filesystem #53
Conversation
| @@ -38,7 +40,7 @@ class aiff_ofstream : public ofstream | |||
| aiff_ofstream( aiff_ofstream&& ); | |||
| aiff_ofstream& operator=( aiff_ofstream&& ); | |||
|
|
|||
| aiff_ofstream( const std::string& file, const info_type& info ); | |||
| aiff_ofstream( const std::filesystem::path& file, const info_type& info ); | |||
There was a problem hiding this comment.
This can break existing code. If a user passes an argument of a type that can implicitly convert to std::string, it might not work if the signature now expects a std::filesystem::path. See https://gcc.godbolt.org/z/49x1vKMEq.
How about overloading instead of replacing?
There was a problem hiding this comment.
Thanks for bringing this up! I didn't consider this tbh. Though I'm slightly more okay with this being a breaking change but that's of course not up to me :)
Would it be acceptable if users that are affected by this adjust like this?
https://gcc.godbolt.org/z/KaTca6o19
There was a problem hiding this comment.
+1 for overloading, not everybody is ready to use std::filesystem, yet. Imho it should mirror the ctors of: https://en.cppreference.com/w/cpp/io/basic_fstream/basic_fstream
In order to fix the bug, it is important that you can pass in a std::wstring instead of a std::string on Windows.
There was a problem hiding this comment.
Heyhey, thanks for the feedback!
In order to fix the bug, it is important that you can pass in a std::wstring instead of a std::string on Windows.
Not sure I agree with the overload though because isn't this exactly what fs::path is trying to solve?
But in any case, just to make sure I understand what you guys are suggesting, it would be something like this?
https://godbolt.org/z/86cPTd95o
In other words: platform macro for string/wstring and additionally a template overload for path because naive overloading doesn't work?
There was a problem hiding this comment.
I guess this is ultimately up to the maintainers, but here's my opinion: I think having a constructor for a std fs path is a good idea. I would try to avoid breaking changes because they might annoy users. In your approach, the point of the template is so that when passing a const char*, the compiler does not consider the fs path ctor?
There was a problem hiding this comment.
Yes, I basically took this from the mentioned fstream overload implementation:
fstream string + path overload
I think in this particular case I disagree with trying to avoid a "breaking" change because:
a) somehow I don't see how it makes sense from a library's perspective to offer a string and a path ctor other than for backwards compatibility reasons but maybe I'm still missing something and
b) it only affects those clients that defined an implicit conversion operator to string for their type and all they'd have to do to fix this would be to change the operator to fs::path. It's actually sort of weird that the compiler can't figure out the transitivity on its own from type -> string -> lib(path) and just needs some slight help here.
But ya, I can only open the PR :)
|
|
||
| namespace audio | ||
| { | ||
|
|
||
| // a map of extension => container type | ||
| using ifstream_container_map = std::map<std::string, ifstream_info::container_type>; | ||
| using ifstream_container_map = std::map<std::filesystem::path, ifstream_info::container_type>; |
There was a problem hiding this comment.
Here I wasn't sure if this might be overkill but motivation was that extensions are also paths.
| @@ -43,34 +43,33 @@ namespace | |||
| bool init() | |||
| { | |||
| #if BOOST_OS_WINDOWS | |||
| boost::filesystem::path::imbue( std::locale( std::locale(), new std::codecvt_utf8_utf16<wchar_t>() ) ); | |||
| std::filesystem::path::imbue( std::locale( std::locale(), new std::codecvt_utf8_utf16<wchar_t>() ) ); | |||
There was a problem hiding this comment.
std::filesystem::path::imbue() does not exist.
It's better to not rely on string conversions, but instead always use the native string format.
There was a problem hiding this comment.
Here I wasn't even sure what this is all about. Could you elaborate why this needed?
There was a problem hiding this comment.
boost::filesystem::path::imbue sets the path locale for boost::filesystem. Using the std::codecvt facet sets up the proper conversion between UTF8 and UTF16 characters. This is needed on Windows for setting up a proper string conversion, as Windows uses a locale based on the system settings. Leaving the locale up to Windows results in conversions when the paths have special characters possibly failing (consider Japanese and Chinese characters on a western system).
Note: std::codecvt has been deprecated so this isn't a futureproof solution.
Exchanges the current use of
std::stringorboost::filesystemwithstd::filesystem::pathas initiated by this issue: #51This is a rather mechanical refactoring and I tried to change as little as possible besides the types.
I also have a follow up PR in the pipeline that does the same for
std::optional, so let me know what you think :)Edit 1:
I didn't do the change in
avassetreader_sourceogg_sourcebecause I wasn't sure if that would be desirable. Please let me know if those should be adjusted as well.