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

#4271: Use OS independent utf8 string representation of std_fs::path at to_j… #4502

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

zel1b08a
Copy link

According to #4271.

std::filesystem::path::string method uses system-dependent native format. String type is always UTF8 format, by the json docs. So we force encoding to be UTF8.

@zel1b08a zel1b08a requested a review from nlohmann as a code owner November 19, 2024 10:31
@github-actions github-actions bot added the S label Nov 19, 2024
Copy link

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @zel1b08a
Please read and follow the Contribution Guidelines.

@nlohmann
Copy link
Owner

Please run make amalgamate with Astyle (http://astyle.sourceforge.net) 3.1 installed.

@@ -435,7 +435,7 @@ inline void to_json(BasicJsonType& j, const T& t)
template<typename BasicJsonType>
inline void to_json(BasicJsonType& j, const std_fs::path& p)
{
j = p.string();
j = p.u8string();
Copy link
Owner

Choose a reason for hiding this comment

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

Note that from C++20, u8string will return std::u8string instead of std::string - I am not sure if this assignment works in this case.

Copy link
Author

@zel1b08a zel1b08a Nov 19, 2024

Choose a reason for hiding this comment

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

I see the copy with reinterpret_cast to char* there only..)

Copy link
Owner

Choose a reason for hiding this comment

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

I saw a remark on cppreference.com

Copy link
Author

Choose a reason for hiding this comment

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

I mean the u8string to string conversion here in that issue

@nlohmann
Copy link
Owner

The exception could come from the conversion of std::u8string to JSON. As std::u8string is not supported, the string is treated like a sequence of bytes and converted to an array.

@coveralls
Copy link

coveralls commented Dec 7, 2024

Coverage Status

coverage: 99.649%. remained the same
when pulling c859d64 on zel1b08a:patch-1
into 9f60e85 on nlohmann:develop.

Copy link

github-actions bot commented Dec 7, 2024

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @zel1b08a
Please read and follow the Contribution Guidelines.

Copy link

github-actions bot commented Dec 9, 2024

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @zel1b08a
Please read and follow the Contribution Guidelines.

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

Successfully merging this pull request may close these issues.

3 participants