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

Introduce comments support #443

Merged
merged 2 commits into from
Oct 14, 2015
Merged

Introduce comments support #443

merged 2 commits into from
Oct 14, 2015

Conversation

andrusha97
Copy link
Contributor

I find support of comments in JSON very useful, especially when one uses JSON as a format for config files. This PR provides a prototype implementation of this feature.
By default comments support is disabled. One should use a new flag kParseCommentsFlag to enable the feature, so this implementation is backward compatible and doesn't weaken the standard compliance by default.


if (is.Peek() == '*') {
is.Take();
while (is.Take() != '*' || is.Take() != '/') { }
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems need to check for null terminator here. Otherwise a malformed input with only open /* may read after null terminator.

* Comments parsing function correctly handles EOF.
* Since SkipWhitespaceAndComments can generate errors, its calls should be followed by RAPIDJSON_PARSE_ERROR_EARLY_RETURN macro.
* Some tests to make the bug never appear again.
@andrusha97
Copy link
Contributor Author

Fixed. Also added some tests to check this bug.

miloyip added a commit that referenced this pull request Oct 14, 2015
Introduce comments support
@miloyip miloyip merged commit a5d9971 into Tencent:master Oct 14, 2015
@miloyip miloyip mentioned this pull request Oct 14, 2015
13 tasks
@miloyip
Copy link
Collaborator

miloyip commented Oct 14, 2015

Thank you.

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