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 consistency in function int_to_string() #2193

Merged
merged 3 commits into from
Jun 24, 2020

Conversation

dota17
Copy link
Contributor

@dota17 dota17 commented Jun 16, 2020

The function int_to_string() call std::to_string(), which will return std::string. For consistency, need to check string_type is std::string.

Refer: #2059

@dota17 dota17 requested a review from nlohmann as a code owner June 16, 2020 07:49
@dota17 dota17 changed the title Fix issue#2059 Fix consistency in function int_to_string() Jun 16, 2020
@coveralls
Copy link

coveralls commented Jun 16, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 0ecf297 on dota17:issue#2059 into e7452d8 on nlohmann:develop.

@nlohmann
Copy link
Owner

What would be the previous behavior if string_type::value_type is not char? I understand now such types are excluded - but what would have happened earlier?

@dota17
Copy link
Contributor Author

dota17 commented Jun 20, 2020

What would be the previous behavior if string_type::value_type is not char? I understand now such types are excluded - but what would have happened earlier?

If char_type is not char, there will be compiler error, the problem will arise before int_to_string, cause the library default string_type is std::string, default char_type is char. So now I might think I may not understand the issue correctly, and so far, I can't used the EASTL to reproduce the issue. The current solution is more of a temporary and partial solution, and more changes may be needed next.

@nlohmann
Copy link
Owner

I am not sure whether this change is improving the situation. Instead, we may change the function to

using std::to_string;
target = to_string(value);

to allow for ADL to choose the right function and the user-defined types to provide these functions.

@dota17
Copy link
Contributor Author

dota17 commented Jun 20, 2020

Thanks. This suggestion is very nice.
And back to the original issue #2059, we may need more information from @bcollins526

@bcollins526
Copy link

Thanks. This suggestion is very nice.
And back to the original issue #2059, we may need more information from @bcollins526

Sure just let me know what you need.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann self-assigned this Jun 23, 2020
@nlohmann nlohmann merged commit 635b9a0 into nlohmann:develop Jun 24, 2020
@nlohmann
Copy link
Owner

Thanks!

@nlohmann
Copy link
Owner


🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

  • ✨ New Feature
  • 🐛 Bug Fix
  • ⚡️ Improvement
  • 🔨 Further Change
  • 🔥 Deprecated function

Description


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

Successfully merging this pull request may close these issues.

None yet

4 participants