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 kParsePartialFractionFlag #961

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gergondet
Copy link

This flag relaxes the requirements on double parsing so that fractions
can begin with a dot or have a dot without fraction digits.

This is related to some of the points in #36

I am not too keen on the flag name but I don't have a more inspired proposal.

This flag relaxes the requirements on double parsing so that fractions
can begin with a dot or have a dot without fraction digits.
@coveralls
Copy link

coveralls commented May 17, 2017

Coverage Status

Coverage increased (+4.0e-05%) to 99.938% when pulling a5f3c17 on gergondet:topic/SupportPartialFractions into 0033268 on miloyip:master.

if (RAPIDJSON_UNLIKELY(!(s.Peek() >= '0' && s.Peek() <= '9'))) {
if (!(parseFlags & kParsePartialFractionFlag))
RAPIDJSON_PARSE_ERROR(kParseErrorNumberMissFraction, s.Tell());
else if (!(s.Peek() == ',' || s.Peek() == '}' || s.Peek() == ']' || s.Peek() == '\0' || s.Peek() == 'e' || s.Peek() == 'E'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should not be needed here. This function should parse as many characters for a JSON number type as it could. It should not involve other characters such as ,}]\0.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for reviewing the code.

I think it is needed to accept a missing fraction.

In the standard case, we should outright reject the number if it does not have numbers following the ..

In the partial fraction case, we can accept the number to finish here (i.e. check for ,}]\0) or to be followed by an exponent but it should not accept anything else.

Without this check, the parser works correctly witht incomplete fractions but the parser fails with kParseErrorDocumentRootNotSingular when parsing .a123 rather than kParseErrorValueInvalid which might be a bit confusing. If you think this is ok, I'll hapilly remove that check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think implementing this relaxed syntax just involve change of the underlying state machine.
For 123.a, it should take 123. and returns a double valued 123.0.
For .123a, it should take .123 and returns a double valued .123.
For .a it should raise a parse error because there must be at least a digit when it is prefixed by ..

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I've removed the extra checks then.

@miloyip
Copy link
Collaborator

miloyip commented Jun 23, 2018

I just realized that the implementation relaxed the syntax such that even . and .E10 are parsed as zero. I think this may be over-relaxed.

I checked ECMAscript's syntax. The decimal number is either (with at least one digit before "." and without digits after), or (without digits before "." and with a least one digit after).

I think we may consider following this syntax, in order to make JSON's syntax relax to ECMAscript's , but not more.

@kkaefer
Copy link
Contributor

kkaefer commented Aug 22, 2019

@miloyip what next actions need to happen to get this merged?

@miloyip
Copy link
Collaborator

miloyip commented Aug 23, 2019

@kkaefer The discussion was suspended... Any idea to the above discussion?

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