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 printing of 4 byte UTF-8 characters #1537

Merged
merged 5 commits into from Aug 20, 2013

Conversation

@aidanhs
Copy link
Contributor

aidanhs commented Aug 18, 2013

Carrying on from the original issue reported at #1382 where I screwed up the conversion to a PR, here is a clean set of commits ready to be pulled.

The only test I ran was python tests/runner.py test_utf.

@juj
juj reviewed Aug 20, 2013
View changes
src/runtime.js Outdated
buffer.push(code);
if (code > 191 && code < 224) {
if (((code & 0xE0) ^ 0xC0) == 0) { // 110xxxxx

This comment has been minimized.

Copy link
@juj

juj Aug 20, 2013

Collaborator

As a (micro(?)-)optimization, could this be written as if ((code & 0xE0) == 0xC0), and the same for the check below on line 404? That would save an xor.

@kripken
Copy link
Member

kripken commented Aug 20, 2013

Please rebase on incoming, so the diff here contains just your new code.

@aidanhs
Copy link
Contributor Author

aidanhs commented Aug 20, 2013

Yeah, sorry about that, sometimes I love git and sometimes it cheerfully permits me to screw myself over...let me see if I can unpick this mess...

@aidanhs
Copy link
Contributor Author

aidanhs commented Aug 20, 2013

Right, fixed.

@juj wasn't too concerned about performance but I do think it's semantically clearer so I like.

@kripken
Copy link
Member

kripken commented Aug 20, 2013

I don't really understand this or utf ;) but looks good and if the previous test passes + a new testcase now works, then excellent. thanks!

kripken added a commit that referenced this pull request Aug 20, 2013
Fix printing of 4 byte UTF-8 characters
@kripken kripken merged commit 6f2c31d into emscripten-core:incoming Aug 20, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.