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

Allow parsing of JSON numbers using number types other than f64 #13869

Closed
wants to merge 1 commit into from
Closed

Allow parsing of JSON numbers using number types other than f64 #13869

wants to merge 1 commit into from

Conversation

zr40
Copy link
Contributor

@zr40 zr40 commented Apr 30, 2014

This change allows numbers in JSON data to be parsed using any number type. With f64, integer values would lose precision once they exceed 2^53.

This is accomplished by changing Number(f64) to Number(~str), storing the input string instead of the parsed f64. The conversion now takes place in .as_number() using the type parameter with FromStr.

Notable changes to tests:

  • Added a case to check reading and writing a big integer without loss of precision.
  • In test_decode_numbers, I've changed 0.4e-01 to 0.5e-01 to avoid one-bit difference between compiled constant and from_str output for the former number.

@lilyball
Copy link
Contributor

I agree that there should be some way to parse JSON numbers that don't fit in f64, but I don't think this is the solution. This is adding heap allocation and cluttering up the usability (as a simple API consumer, Number(~str) makes no sense to me. I want a number!)

@lilyball
Copy link
Contributor

As discussed on IRC, I think there's room here to have the numeric type be generic, e.g. pub enum JSON<NumType = f64> { Number(NumType), .. }.

@alexcrichton
Copy link
Member

I would be worried that a default type parameter on the json type is becoming a bit too generic. If I can control the number type, why can't I control the string type? (e.g. a small string).

Is this in line with the JSON spec? Looking around this seems to indicate that this is what JSON does.

@erickt
Copy link
Contributor

erickt commented May 1, 2014

What if instead we remove the Number variant, and replace it with Integer and Floating variants? This is how Doug Crockford's JSON-java library handles this issue.

@lilyball
Copy link
Contributor

lilyball commented May 1, 2014

@alexcrichton JSON does not define the semantics of parsing, only the grammar. A fair number of JSON parsers uses f64 for numbers, because this is how JavaScript works, but some of them support arbitrary-precision decimal numbers.

@erickt That's a good idea. That way we could trivially support u64 without additional complexity. I think there's utility in being able to support bigints, but I'm not sure the best way to do that without introducing additional complexity. Using the type parameter for that may work, but it's not a great solution.

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to reopen with a rebase!

@zr40 zr40 deleted the json-numbers branch February 23, 2015 20:41
arcnmx pushed a commit to arcnmx/rust that referenced this pull request Jan 9, 2023
Improve exit point highlighting for non-loop loops in tail position
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