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

Fix std::filesystem::path Database constructor (fixes #317) #319

Closed
wants to merge 1 commit into from

Conversation

Chronial
Copy link

@Chronial Chronial commented Jan 8, 2021

The return type of path.c_str() is platform-dependent and is a wstring on windows. path.u8string() always returns an utf8-encoded string.

In c++17, that string has type std::string, but starting in c++20 it is std::u8string. I used a reinterpret_cast to handle the c++20 case.

Alternatively, a constructor taking u8string could be added for the c++20 case. But it seems odd to have only one place where the library accepts u8strings. If support for u8strings is added, it should probably be done for the whole library at once.

The return value of path.c_str() is platform-dependent and is a wstring
on windows. path.u8string() always returns an utf8-encoded string.
In c++17, that string has type std::string, but starting in c++20 it is
std::u8string. We use a reinterpret_cast to handle the c++20 case.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 42ac337 on Chronial:feature/fs_path into ef650e0 on SRombauts:master.

@chronoxor
Copy link
Contributor

chronoxor commented Jan 8, 2021

I confirm that the fix works well for VS2019, but not for MinGW64 8.1.0 due to - https://stackoverflow.com/questions/50890493/compile-error-when-stdfilesystem-header-file-is-added-to-my-program

I add more complex compiler checking:

// c++17: MinGW GCC version > 8
// c++17: Visual Studio 2017 version 15.7
#if ((__cplusplus >= 201703L) && ((!defined(__MINGW32__) && !defined(__MINGW64__)) || (__GNUC__ > 8))) || (defined(_MSVC_LANG) && (_MSVC_LANG >= 201703L))

@SRombauts
Copy link
Owner

Hello, I believe that this is not needed anymore now that I merged the other ones, right?
I'll close it but let me know if something is still incorrect

@SRombauts SRombauts closed this Jan 18, 2021
@Chronial
Copy link
Author

Yes, this was already merged as part of #318

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.

4 participants