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

Add doubleNonFinite for IEEE 754 floats #5

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

Conversation

velveteer
Copy link
Owner

@velveteer velveteer commented Nov 6, 2021

Addresses #3

@velveteer velveteer force-pushed the add-double-non-finite branch 2 times, most recently from fcf418c to 196c0c1 Compare November 7, 2021 20:59
@velveteer velveteer force-pushed the master branch 2 times, most recently from 37bd7c0 to 0e32505 Compare November 7, 2021 21:04
Comment on lines +549 to +538
getDoubleNonFinite :: Value -> Decoder Double
getDoubleNonFinite = withRawByteString $ \bs ->
case BSC.strip bs of
"\"+inf\"" -> pure $ 1/0
"\"-inf\"" -> pure $ (-1)/0
"null" -> pure $ 0/0
_ -> Sci.toRealFloat <$> parseScientific bs
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for quickly developing this PR.

I noticed that it matches on the actual bytestring inside the JSON AST. This means that if the string is encoded e.g. using escape codes, it wouldn't parse. Granted, this is a very academic issue, but I thought it was worth pointing out.

For example, using Aeson:

ghci> decode @Double "\"\\u002dinf\""
Just (-Infinity)

But using this PR:

ghci> decodeEither personDecoder "{\"lol\": \"\\u002dinf\"}"
Left (InternalException (HError {path = "/lol", errorMsg = "Failed to parse Scientific: Failed reading: takeWhile1", docLocation = "\"\\u002dinf\"}", docDebug = "json_iterator [ depth : 2, structural : '\"', offset : 8', error : No error ]"}))
ghci> decodeEither personDecoder "{\"lol\": \"-inf\"}"
Right (Person (-Infinity))

I don't really mind too much, I think the PR is fine as is. I don't think it is likely that people will re-encode their Aeson JSON such that it uses escape codes for the non-finite IEEE 754 values.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah that's the trick with get_raw_json_token, whereas get_string escapes the string but will generate an exception if the value isn't actually a string. I will see if I can address this without losing performance. Although I do agree not handling escaped strings for this particular decoder should not be a major problem.

@velveteer velveteer force-pushed the add-double-non-finite branch from 196c0c1 to 3bb972f Compare November 9, 2021 20:22
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.

2 participants