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 Parsing Special Numbers #1391

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

Conversation

Alexhuszagh
Copy link
Contributor

Closes #1384, by adding logic to specially handle special floating-point numbers. This should correctly handle both streaming and complete parsers, and the unittests seem to confirm this, but a double-check of my logic would be appreciated.

Copy link
Contributor

@Stargateur Stargateur left a comment

Choose a reason for hiding this comment

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

Most comment are the same for float double streaming and complete version

src/number/streaming.rs Outdated Show resolved Hide resolved
src/number/complete.rs Outdated Show resolved Hide resolved
src/number/streaming.rs Outdated Show resolved Hide resolved
src/number/streaming.rs Outdated Show resolved Hide resolved
src/number/streaming.rs Outdated Show resolved Hide resolved
src/number/streaming.rs Outdated Show resolved Hide resolved
src/number/complete.rs Outdated Show resolved Hide resolved
src/number/complete.rs Outdated Show resolved Hide resolved
@Alexhuszagh Alexhuszagh changed the title Fixed #1384, by adding logic to specially handle special floating-point numbers. This should correctly handle both streaming and complete parsers, and the unittests seem to confirm this, but a double-check of my logic would be appreciated. Fix Parsing Special Numbers Sep 10, 2021
src/number/complete.rs Outdated Show resolved Hide resolved
src/number/complete.rs Outdated Show resolved Hide resolved
@Alexhuszagh
Copy link
Contributor Author

@Stargateur Speaking of which, I'm guessing it's a good idea to not mark any internal functions public? I did this somewhat because nom does it a lot internally, but it's probably not a good idea...

@Stargateur
Copy link
Contributor

Stargateur commented Sep 10, 2021

@Stargateur Speaking of which, I'm guessing it's a good idea to not mark any internal functions public? I did this somewhat because nom does it a lot internally, but it's probably not a good idea...

Oh, well, I think unless someone have a good reason yeah we should not introduce too many pub item.

@Alexhuszagh
Copy link
Contributor Author

Ok I believe every requested change has been made.

($input:ident, $t:ident) => {{
let (input, sign) = sign($input.clone())?;
let b = input.as_bytes();
let (mut float, count) = if b.len() >= 3 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will not parse correctly in streaming: If we get Na as partial input it will return an error instead of Incomplete (once we refill the buffer, the next character could be N and we get NaN).
The Compare trait already handles those cases

@Geal Geal added this to the 8.0 milestone Mar 14, 2022
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.

Updating to Nom 7 has broken double("NaN")
3 participants