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-regexp.cc
File src/runtime/runtime-regexp.cc (right):
https://codereview.chromium.org/788043005/diff/220001/src/runtime/runtime-regexp.cc#newcode825
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/unicode-escapes-in-regexps.js
File test/mjsunit/harmony/unicode-escapes-in-regexps.js (right):
https://codereview.chromium.org/788043005/diff/220001/test/mjsunit/harmony/unicode-escapes-in-regexps.js#newcode145
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/unicode-escapes-in-regexps.js#newcode212
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.
https://codereview.chromium.org/788043005/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
