Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

Fix #38 #145

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix #38 #145

wants to merge 1 commit into from

Conversation

rominf
Copy link

@rominf rominf commented Dec 12, 2019

Parse the number according to the JSON grammar, ignoring the current
locale.

Parse the number according to the JSON grammar, ignoring the current
locale.
Copy link
Contributor

@artwyman artwyman left a comment

Choose a reason for hiding this comment

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

#38 is about string output not parsing, though the problem seems to go both ways. A complete solution would need to handle both

* Parse the number according to the JSON grammar, ignoring the current locale assuming that the
* string is already validated to comply with the JSON grammar.
*/
double strtod_dot(const char* str) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a good approach, but I don't see it called anywhere. You probably meant to call it in parse_number().

@@ -252,6 +252,18 @@ JSON11_TEST_CASE(json11_test) {

}

JSON11_TEST_CASE(json11_test_numbers) {
const std::string numbers =
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to override the locale in order to really test what you intend.

const std::string numbers_json = Json::parse(numbers, err).dump();
std::cout << "numbers_json: " << numbers_json << std::endl;
JSON11_TEST_ASSERT(numbers_json == expected_numbers);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also test round-trip back to string to ensure no locale issues there.

@artwyman artwyman added the bug label Jan 27, 2020
@artwyman
Copy link
Contributor

Thanks for your effort! This repo is no longer supported by Dropbox and about to be archived. I'm not going to try to merge this PR at the last minute, but I'll leave it open for reference by anyone who encounters the same problem.

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

Successfully merging this pull request may close these issues.

2 participants