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

parser: parse Unicode escape sequences in keys #219

Closed
wants to merge 4 commits into from

Conversation

belochub
Copy link
Member

@belochub belochub commented Jun 12, 2017

Also, add support for Unicode surrogate pairs and improve key parsing performance by avoiding copying when possible.
Refs: https://github.com/metarhia/jstp/issues/152

@belochub belochub added this to the 1.0.0 milestone Jun 12, 2017
@aqrln
Copy link
Member

aqrln commented Jun 15, 2017

@belochub can you please rebase it? I think removing the old package-lock.json, resolving the conflict in package.json and regenerating the package-lock should be just fine.

Copy link
Member

@nechaido nechaido left a comment

Choose a reason for hiding this comment

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

LGTM

@belochub belochub added blocked and removed blocked labels Jun 16, 2017
@nechaido
Copy link
Member

@belochub can you, please, rebase this PR on master?

@belochub
Copy link
Member Author

@aqrln, @lundibundi, ping.

Copy link
Member

@lundibundi lundibundi left a comment

Choose a reason for hiding this comment

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

LGTM.


if (isxdigit(str[0])) {
result = ReadHexNumber(str, 4, ok);
*size = 4;
Copy link
Member

Choose a reason for hiding this comment

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

Should it check whether *ok is true before changing *size?

return 0xFFFD;
}
}
result = ReadHexNumber(str + 1, hex_size, ok);
Copy link
Member

Choose a reason for hiding this comment

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

It should check whether result is a valid code point (i.e., something like \u{1234123412341234} must be a syntax error).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I looked into it and decided to reimplement ReadHexNumber overall, because it has some other problems.

@belochub
Copy link
Member Author

@aqrln, I've updated ReadHexNumber function to make it safer and added tests for it, PTAL.
Also, this probably fixed parsing of strings like this: '\u00001' with the digits after the escape sequence, this case was incorrectly considered an error before.

aqrln pushed a commit that referenced this pull request Jun 26, 2017
* Implement surrogate Unicode pairs parsing.
* Parse Unicode escape sequences in objects.
* Rework ReadHexNumber function.
* Improve key parsing performance by avoiding copying when possible.

PR-URL: #219
Refs: https://github.com/metarhia/jstp/issues/152
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@aqrln
Copy link
Member

aqrln commented Jun 26, 2017

Landed in bef3572.

@aqrln aqrln closed this Jun 26, 2017
@aqrln aqrln deleted the parser-unicode-escapes branch June 26, 2017 14:15
belochub added a commit that referenced this pull request Jan 22, 2018
* Implement surrogate Unicode pairs parsing.
* Parse Unicode escape sequences in objects.
* Rework ReadHexNumber function.
* Improve key parsing performance by avoiding copying when possible.

PR-URL: #219
Refs: https://github.com/metarhia/jstp/issues/152
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
belochub added a commit that referenced this pull request Jan 22, 2018
* Implement surrogate Unicode pairs parsing.
* Parse Unicode escape sequences in objects.
* Rework ReadHexNumber function.
* Improve key parsing performance by avoiding copying when possible.

PR-URL: #219
Refs: https://github.com/metarhia/jstp/issues/152
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@belochub belochub mentioned this pull request Jan 22, 2018
belochub added a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018
* Implement surrogate Unicode pairs parsing.
* Parse Unicode escape sequences in objects.
* Rework ReadHexNumber function.
* Improve key parsing performance by avoiding copying when possible.

PR-URL: metarhia/jstp#219
Refs: https://github.com/metarhia/jstp/issues/152
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
belochub added a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018
* Implement surrogate Unicode pairs parsing.
* Parse Unicode escape sequences in objects.
* Rework ReadHexNumber function.
* Improve key parsing performance by avoiding copying when possible.

PR-URL: metarhia/jstp#219
Refs: https://github.com/metarhia/jstp/issues/152
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
belochub added a commit to metarhia/mdsf that referenced this pull request Jul 21, 2018
* Implement surrogate Unicode pairs parsing.
* Parse Unicode escape sequences in objects.
* Rework ReadHexNumber function.
* Improve key parsing performance by avoiding copying when possible.

PR-URL: metarhia/jstp#219
Refs: https://github.com/metarhia/jstp/issues/152
Reviewed-By: Dmytro Nechai <nechaido@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants