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.

Sign up for GitHub

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

Jump to bottom

Recognize identifier characters from the Supplementary Multilingual Plane #1245

Closed
wants to merge 1 commit into from
Closed

Recognize identifier characters from the Supplementary Multilingual Plane #1245

wants to merge 1 commit into from

Conversation

ariya
Copy link
Contributor

@ariya ariya commented Jul 21, 2015

Fixes #1244.

@ariya
Copy link
Contributor Author

ariya commented Jul 21, 2015

Ugh, I missed some additional test files. Stay tuned...

@ariya
Copy link
Contributor Author

ariya commented Jul 21, 2015

The failure on Travis CI is due to the failing downstream tests with JSCS.

@mikesherov Seems that JSCS master doesn't really pass, i.e. the following:

git clone https://github.com/jscs-dev/node-jscs.git
cd node-jscs
npm install
npm test

fails for me (Node.js v0.12.0, npm 2.9.1).

@mikesherov
Copy link
Member

Let's skip jscs master for now.

@ariya
Copy link
Contributor Author

ariya commented Jul 22, 2015

@mikesherov I'll commit something quickly on master to skip JSCS, so that any PRs will be unblocked.

@ariya ariya mentioned this pull request Jul 22, 2015
Closed
@ariya
Copy link
Contributor Author

ariya commented Jul 23, 2015

@ikarienator @mikesherov @michaelficarra @mathiasbynens Anyone willing to review this?

mathiasbynens
mathiasbynens reviewed Jul 23, 2015
View reviewed changes
esprima.js
if (cp >= 0xD800 && cp <= 0xDBFF) {
first = cp;
second = source.charCodeAt(i + 1);
cp = (first - 0xD800) * 0x400 + second - 0xDC00 + 0x10000;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What about lone high surrogates? E.g. \uD800 followed by x.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I'll make sure that will trigger an error.

@mikesherov
Copy link
Member

I completely defer to @mathiasbynens's expertise in this area.

@ariya
Copy link
Contributor Author

ariya commented Jul 28, 2015

Anything else I should worry about before I am checking this in?

@mathiasbynens
Copy link
Contributor

I’ve collected some edge cases that might make for interesting tests here: https://mathiasbynens.be/notes/javascript-identifiers-es6 But the patch looks good to me as-is.

@ariya
Copy link
Contributor Author

ariya commented Jul 28, 2015

@mathiasbynens I think I've covered most, if not all, of the corner cases you've described. Very helpful, thank you so much for that!

@ariya ariya closed this in 82e5030 Jul 28, 2015
@ariya ariya deleted the smp branch August 12, 2015 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ariya @mikesherov @mathiasbynens @jquerybot