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 issue740: parse colon in plain scalar correctly #911

Merged
merged 2 commits into from
Jul 2, 2020

Conversation

dota17
Copy link
Collaborator

@dota17 dota17 commented Jul 1, 2020

According to the yaml specification, a colon can be seen as a part of a plain scalar if it is not followed by white space in most cases.

(RegEx(':') + (BlankOrBreak() | RegEx() | RegEx(",]}", REGEX_OR))) |

In function EndScalarInFlow() , RegEx(':')+RegEx(",]}", REGEX_OR) means if a ":" is followed by a "," or " ] " or " }", the ":" will not be a part of a plain scalar. It is against the spec, so I removed it, and this solved the problem of #740 and #899.

*ex7_3 and *ex7_17 in specexamples.h have a little difference with the spec content, This difference will cause the corresponding testcases can't work as expected because of the new rule. I changed them to be the same as spec.

@perlpunk
Copy link

perlpunk commented Jul 1, 2020

The spec ns-plain-char says:

[129] ns-plain-safe-in ::= ns-char - c-flow-indicator
[130] ns-plain-char(c) ::= ( ns-plain-safe(c) - “:” - “#” )
                         | ( /* An ns-char preceding */ “#” )
                         | ( “:” /* Followed by an ns-plain-safe(c) */ )

That means, I believe, that for example a colon followed by a comma or another flow-indicator will end a plain scalar in flow.
So, as far as I understand, this PR will make this example (7.17) invalid (you added spaces in test/specexamples.h):

{
unquoted : "separate",
http://foo.com,
omitted value:,  # colon + comma
: omitted key,
}

But this is valid (it's an example in the spec after all), and part of the YAML Test Suite:
https://github.com/yaml/yaml-test-suite/blob/master/test/4ABK.tml

@dota17
Copy link
Collaborator Author

dota17 commented Jul 1, 2020

Ok, I see. @perlpunk ,thanks for your explaination. I have a misunderstanding about this and confuse the “°” with “·”. I will close this PR.

@dota17 dota17 closed this Jul 1, 2020
@@ -641,8 +641,8 @@ const char *ex7_17 =
"{\n"
"unquoted : \"separate\",\n"
"http://foo.com,\n"
"omitted value:,\n"
": omitted key,\n"
"omitted value: ,\n"
Copy link

Choose a reason for hiding this comment

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

If you have a closer look at the spec example 7.17, you'll notice that it contains

omitted value:°,

where the ° stands for the empty node, so it should not be replaced with a space.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ha-ha, yes. And if one is not careless, it's easy to confuse them, just like me.

@perlpunk
Copy link

perlpunk commented Jul 1, 2020

It would be cool to fix #899 though.
When I ran that recently in yaml-runtimes (generating the https://matrix.yaml.io/) it froze my computer, and I had to hard reset it, so I'm seriously considering taking out yaml-cpp from the test matrix :-/

@dota17
Copy link
Collaborator Author

dota17 commented Jul 1, 2020

ok, I will solve this problem as soon as possible

@dota17
Copy link
Collaborator Author

dota17 commented Jul 1, 2020

@perlpunk Should processor throw an exception when it parses the [:]? Or, parse it as [~: ~]. I know that pyyaml dose throw an exception.

@perlpunk
Copy link

perlpunk commented Jul 1, 2020

@dota17 it is actually an edge case.
The reference parser and HsYAML and a couple of other libraries parse that the same as [ null : null ], but since it is an edge case and noone should write YAML like that, I think it's not that important what the result is, as long as it does not hang.

Edit: and as an additional note, empty implicit keys will very probably be removed in the next YAML version, so this case will become invalid anyway.

@dota17 dota17 reopened this Jul 2, 2020
@dota17
Copy link
Collaborator Author

dota17 commented Jul 2, 2020

Update the solution and testcases for the bug in issue741/899. The previous solution has been deleted.

@jbeder jbeder merged commit 026a53f into jbeder:master Jul 2, 2020
@jbeder jbeder mentioned this pull request Jul 2, 2020
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.

3 participants