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 example for hexadecimal escape #35

Merged
merged 1 commit into from
Jul 6, 2015
Merged

Conversation

bergus
Copy link
Contributor

@bergus bergus commented Jul 6, 2015

Follow up to #34.
@anba I knew it, damn.

The previous text was based on fiddling around in a buggy implementation.
benjamingr added a commit that referenced this pull request Jul 6, 2015
fix example for hexadecimal escape
@benjamingr benjamingr merged commit de7b32f into tc39:master Jul 6, 2015
@benjamingr
Copy link
Collaborator

Thanks

@domenic
Copy link
Member

domenic commented Jul 6, 2015

This still doesn't make much sense. new RegExp("\\u004" + "A").test("u004A") === false.

@mathiasbynens
Copy link
Member

I don’t understand the confusion.

RegExp('\\u004' + 'A').test('J');
// → true

RegExp('\\u004' + 'A', 'u').test('J');
// → true

This patch makes sure that:

RegExp('\\u004' + RegExp.escape('A')).test('J');
// → false

RegExp('\\u004' + RegExp.escape('A'), 'u').test('J');
// → throws

@anba
Copy link

anba commented Jul 6, 2015

If it's required (or desirable) to prevent RegExp('\\u004' + RegExp.escape('A')) expanding to RegExp('\\u004A'), then you probably also want to make sure RegExp('\\c' + RegExp.escape('G')) does not expand to RegExp('\\cG'). That means not only 0-9A-Fa-f, but actually 0-9A-Za-z_ needs to be escaped.

@bergus
Copy link
Contributor Author

bergus commented Jul 6, 2015

@anba Can you add that to #29, please? I didn't think /\c/ alone was valid, but it seems to compile indeed (though I don't see what it matches).

@anba
Copy link

anba commented Jul 6, 2015

@bergus Added a comment to the other issue.

@bergus bergus deleted the patch-1 branch July 6, 2015 16:36
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.

5 participants