ES6 unicode escapes, part 2: Regexps.

ES6 unicode escapes, part 2: Regexps.

Allows \u{xxxxx} in regexps. Behind the --harmony-unicode flag.

Part 1 is here: https://codereview.chromium.org/716423002

BUG=v8:3648
LOG=N

Committed: https://crrev.com/2305cfb4e2b196918095d3404267772e93916616
Cr-Commit-Position: refs/heads/master@{#26018}

[marja, 2015-01-08 12:03:49]

marja@chromium.org changed reviewers:
+ rossberg@chromium.org

[marja, 2015-01-08 12:05:01]

rossberg, ptal

arv, matthias, caitpotter88, feel free to have a look too

[mathias, 2015-01-08 12:29:06]

mathias@qiwi.be changed reviewers:
+ mathias@qiwi.be

[mathias, 2015-01-08 12:29:07]

https://codereview.chromium.org/788043005/diff/220001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/788043005/diff/220001/src/parser.cc#newcode4354
src/parser.cc:4354: }
Should `-` be a “syntax character” as well because of its special meaning within
character classes (e.g. `/[a-b]/`)?

https://codereview.chromium.org/788043005/diff/220001/src/regexp.js
File src/regexp.js (right):

https://codereview.chromium.org/788043005/diff/220001/src/regexp.js#newcode28
src/regexp.js:28: flags += (pattern.unicode ? 'u' : '');
`u` goes before `y`.

https://codereview.chromium.org/788043005/diff/220001/src/regexp.js#newcode241
src/regexp.js:241: if (harmony_unicode && this.unicode) result += 'u';
Same here.

https://codereview.chromium.org/788043005/diff/220001/test/mjsunit/harmony/un...
File test/mjsunit/harmony/unicode-escapes-in-regexps.js (right):

https://codereview.chromium.org/788043005/diff/220001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unicode-escapes-in-regexps.js:212: })();
How about adding a test for `/[a-c\-]/u`, given the above comment?

Also you may want to update `test/mjsunit/harmony/regexp-flags.js` so it
includes `u`.

[rossberg, 2015-01-08 12:47:45]

lgtm

https://codereview.chromium.org/788043005/diff/220001/src/runtime/runtime-reg...
File src/runtime/runtime-regexp.cc (right):

https://codereview.chromium.org/788043005/diff/220001/src/runtime/runtime-reg...
src/runtime/runtime-regexp.cc:825: case 'u':
Nit: sort cases alphabetically

https://codereview.chromium.org/788043005/diff/220001/test/mjsunit/harmony/un...
File test/mjsunit/harmony/unicode-escapes-in-regexps.js (right):

https://codereview.chromium.org/788043005/diff/220001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unicode-escapes-in-regexps.js:145: new
RegExp("\\u{110000}")
Perhaps add the case

assertThrows{"new RegExp('\\\\u{1f}')", SyntaxError)

[marja, 2015-01-08 13:42:19]

Thanks for comments!

https://codereview.chromium.org/788043005/diff/220001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/788043005/diff/220001/src/parser.cc#newcode4354
src/parser.cc:4354: }
On 2015/01/08 12:29:07, mathias wrote:
> Should `-` be a “syntax character” as well because of its special meaning
within
> character classes (e.g. `/[a-b]/`)?

The spec (draft rev 30) says:

SyntaxCharacter :: one of
^ $ \ . * + ? ( ) [ ] { } |

- loses its special meaning if it's the first or the last character in the
character class, so [a-b-] matches a, b, and -.

But idk, hard to say if it's an omission in the spec or intentional. I filed a
bug to ask that: https://bugs.ecmascript.org/show_bug.cgi?id=3519

If they update the spec, I'll update the implementation.

https://codereview.chromium.org/788043005/diff/220001/src/regexp.js
File src/regexp.js (right):

https://codereview.chromium.org/788043005/diff/220001/src/regexp.js#newcode28
src/regexp.js:28: flags += (pattern.unicode ? 'u' : '');
On 2015/01/08 12:29:07, mathias wrote:
> `u` goes before `y`.

Done.

https://codereview.chromium.org/788043005/diff/220001/src/regexp.js#newcode241
src/regexp.js:241: if (harmony_unicode && this.unicode) result += 'u';
On 2015/01/08 12:29:07, mathias wrote:
> Same here.

Done + updated mjsunit/harmony/regexp-flags.js to ensure that the flags are
alphabetized correctly.

https://codereview.chromium.org/788043005/diff/220001/src/runtime/runtime-reg...
File src/runtime/runtime-regexp.cc (right):

https://codereview.chromium.org/788043005/diff/220001/src/runtime/runtime-reg...
src/runtime/runtime-regexp.cc:825: case 'u':
On 2015/01/08 12:47:45, rossberg wrote:
> Nit: sort cases alphabetically

Done.

https://codereview.chromium.org/788043005/diff/220001/test/mjsunit/harmony/un...
File test/mjsunit/harmony/unicode-escapes-in-regexps.js (right):

https://codereview.chromium.org/788043005/diff/220001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unicode-escapes-in-regexps.js:145: new
RegExp("\\u{110000}")
On 2015/01/08 12:47:45, rossberg wrote:
> Perhaps add the case
> 
> assertThrows{"new RegExp('\\\\u{1f}')", SyntaxError)

That doesn't throw even w/ the current implementation.

Looks like the thing inside { } doesn't need to be a number, if it's not, it's
just not treated as a count specifier.

https://codereview.chromium.org/788043005/diff/220001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unicode-escapes-in-regexps.js:212: })();
On 2015/01/08 12:29:07, mathias wrote:
> How about adding a test for `/[a-c\-]/u`, given the above comment?
> 
> Also you may want to update `test/mjsunit/harmony/regexp-flags.js` so it
> includes `u`.

I'll do the first part if the spec gets updated, second part done.

[rossberg, 2015-01-08 14:11:46]

https://codereview.chromium.org/788043005/diff/220001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/788043005/diff/220001/src/parser.cc#newcode4354
src/parser.cc:4354: }
On 2015/01/08 13:42:18, marja wrote:
> On 2015/01/08 12:29:07, mathias wrote:
> > Should `-` be a “syntax character” as well because of its special meaning
> within
> > character classes (e.g. `/[a-b]/`)?
> 
> The spec (draft rev 30) says:
> 
> SyntaxCharacter :: one of
> ^ $ \ . * + ? ( ) [ ] { } |
> 
> - loses its special meaning if it's the first or the last character in the
> character class, so [a-b-] matches a, b, and -.
> 
> But idk, hard to say if it's an omission in the spec or intentional. I filed a
> bug to ask that: https://bugs.ecmascript.org/show_bug.cgi?id=3519
> 
> If they update the spec, I'll update the implementation.

It's not a SyntaxCharacter and that's likely intentional: it has no special
status outside a character class, and inside you can put it first or last, so
you don't need to escape it in either case.

https://codereview.chromium.org/788043005/diff/220001/test/mjsunit/harmony/un...
File test/mjsunit/harmony/unicode-escapes-in-regexps.js (right):

https://codereview.chromium.org/788043005/diff/220001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unicode-escapes-in-regexps.js:145: new
RegExp("\\u{110000}")
On 2015/01/08 13:42:19, marja wrote:
> On 2015/01/08 12:47:45, rossberg wrote:
> > Perhaps add the case
> > 
> > assertThrows{"new RegExp('\\\\u{1f}')", SyntaxError)
> 
> That doesn't throw even w/ the current implementation.
> 
> Looks like the thing inside { } doesn't need to be a number, if it's not, it's
> just not treated as a count specifier.

Hm, that looks like a spec deviation then, as I don't see anything in the
grammar allowing it. But that's clearly a separate issue then.

[mathias, 2015-01-08 17:12:56]

LGTM.

https://codereview.chromium.org/788043005/diff/190011/src/regexp.js
File src/regexp.js (right):

https://codereview.chromium.org/788043005/diff/190011/src/regexp.js#newcode241
src/regexp.js:241: if (harmony_unicode && this.unicode) result += 'u';
Let’s move this up one line so `u` goes before `y` here as well, for
consistency.

[marja, 2015-01-09 12:11:58]

https://codereview.chromium.org/788043005/diff/190011/src/regexp.js
File src/regexp.js (right):

https://codereview.chromium.org/788043005/diff/190011/src/regexp.js#newcode241
src/regexp.js:241: if (harmony_unicode && this.unicode) result += 'u';
On 2015/01/08 17:12:56, mathias wrote:
> Let’s move this up one line so `u` goes before `y` here as well, for
> consistency.

Done.

[marja, 2015-01-09 12:12:30]

since the sticky flag was added to the regexp mirror, i'm adding the unicode
flag too in the latest-1 patch set.

[mathias, 2015-01-09 12:14:52]

On 2015/01/09 12:12:30, marja wrote:
> since the sticky flag was added to the regexp mirror, i'm adding the unicode
> flag too in the latest-1 patch set.

Let’s update test/mjsunit/mirror-regexp.js too then?

\u{1F44D} Thanks for your amazing work on this, Marja! \u{1F44D}

[marja, 2015-01-09 16:37:47]

marja@chromium.org changed reviewers:
+ yangguo@google.com

[marja, 2015-01-09 16:37:54]

Updated the test, though, it's not very exciting (just testing properties, and
also passes without the corresponding code change - what I gathered from
yangguo@ is that the mirror regexps don't really affect anything for realz).

yangguo, could you review the mirror changes (impl + test)?

[marja, 2015-01-09 17:15:19]

Oh, and, mathias, thx, also, this is very cool:
https://mathiasbynens.be/notes/es6-unicode-regex \u{263A}

[Yang, 2015-01-12 09:16:02]

On 2015/01/09 17:15:19, marja wrote:
> Oh, and, mathias, thx, also, this is very cool:
> https://mathiasbynens.be/notes/es6-unicode-regex \u{263A}

regexp mirror lgtm.

[marja, 2015-01-12 09:22:06]

The CQ bit was checked by marja@chromium.org

[commit-bot: I haz the power, 2015-01-12 09:23:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/788043005/310001

[commit-bot: I haz the power, 2015-01-12 09:50:26]

Committed patchset #17 (id:310001)

[commit-bot: I haz the power, 2015-01-12 09:50:50]

Patchset 17 (id:??) landed as
https://crrev.com/2305cfb4e2b196918095d3404267772e93916616
Cr-Commit-Position: refs/heads/master@{#26018}

[Erik Corry Chromium.org, 2015-01-12 12:10:21]

erikcorry@chromium.org changed reviewers:
+ erikcorry@chromium.org

[Erik Corry Chromium.org, 2015-01-12 12:10:23]

https://codereview.chromium.org/788043005/diff/310001/test/mjsunit/harmony/un...
File test/mjsunit/harmony/unicode-escapes-in-regexps.js (right):

https://codereview.chromium.org/788043005/diff/310001/test/mjsunit/harmony/un...
test/mjsunit/harmony/unicode-escapes-in-regexps.js:54: testRegexpHelper(new
RegExp("[\u{62}-\u{00000066}]oo", "u"));
Sorry I'm late with this feedback, but it would have been nice to have a test
for a \u escape in the range 0x80 to 0xffff.

[mathias, 2015-01-21 07:16:54]

https://codereview.chromium.org/788043005/diff/220001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/788043005/diff/220001/src/parser.cc#newcode4354
src/parser.cc:4354: }
/[\-]/u is now allowed: https://bugs.ecmascript.org/show_bug.cgi?id=3519#c1