Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(64)

Issue 770333005: Implement the `RegExp.prototype.flags` getter (Closed)

Created:
4 years, 7 months ago by mathiasb
Modified:
4 years, 7 months ago
CC:
v8-dev
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Project:
v8
Visibility:
Public.

Description

Implement the `RegExp.prototype.flags` getter TEST=mjsunit/harmony BUG=v8:3751 LOG=N

Patch Set 1 #

Total comments: 2

Patch Set 2 : Move to separate harmony-regexp.js file #

Total comments: 8

Patch Set 3 : Updated as per feedback #

Patch Set 4 : #

Patch Set 5 : #

Total comments: 3

Patch Set 6 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+82 lines, -3 lines) Patch
M BUILD.gn View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M src/bootstrapper.cc View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
A src/harmony-regexp.js View 1 2 3 4 5 1 chunk +35 lines, -0 lines 0 comments Download
M src/messages.js View 1 2 1 chunk +1 line, -0 lines 0 comments Download
A test/mjsunit/harmony/regexp-flags.js View 1 2 3 4 5 1 chunk +40 lines, -0 lines 0 comments Download
M tools/gyp/v8.gyp View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 22 (3 generated)
mathias
PTAL https://codereview.chromium.org/770333005/diff/1/src/regexp.js File src/regexp.js (right): https://codereview.chromium.org/770333005/diff/1/src/regexp.js#newcode456 src/regexp.js:456: //} Uncommenting the `if` statement breaks the build. ...
4 years, 7 months ago (2014-12-09 22:50:48 UTC) #2
Dmitry Lomov (no reviews)
https://codereview.chromium.org/770333005/diff/1/src/regexp.js File src/regexp.js (right): https://codereview.chromium.org/770333005/diff/1/src/regexp.js#newcode452 src/regexp.js:452: //if (harmony_regexps) { harmony_regexps should not be used in ...
4 years, 7 months ago (2014-12-09 23:02:00 UTC) #4
mathias
On 2014/12/09 23:02:00, Dmitry Lomov (chromium) wrote: > https://codereview.chromium.org/770333005/diff/1/src/regexp.js > File src/regexp.js (right): > > ...
4 years, 7 months ago (2014-12-09 23:23:36 UTC) #5
mathias
https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/regexp-flags.js File test/mjsunit/harmony/regexp-flags.js (right): https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/regexp-flags.js#newcode27 test/mjsunit/harmony/regexp-flags.js:27: .get.call(object); Nit: +2 indent Will fix in the next ...
4 years, 7 months ago (2014-12-09 23:26:22 UTC) #6
Dmitry Lomov (no reviews)
https://codereview.chromium.org/770333005/diff/20001/src/harmony-classes.js File src/harmony-classes.js (right): https://codereview.chromium.org/770333005/diff/20001/src/harmony-classes.js#newcode21 src/harmony-classes.js:21: ['Function.prototype.toMethod', Looks like this is from some other patch? ...
4 years, 7 months ago (2014-12-09 23:39:13 UTC) #7
arv (Not doing code reviews)
https://codereview.chromium.org/770333005/diff/20001/src/harmony-regexp.js File src/harmony-regexp.js (right): https://codereview.chromium.org/770333005/diff/20001/src/harmony-regexp.js#newcode15 src/harmony-regexp.js:15: var RegExpGetFlags = function() { Move this out of ...
4 years, 7 months ago (2014-12-10 02:54:50 UTC) #8
mathias
https://codereview.chromium.org/770333005/diff/20001/src/harmony-classes.js File src/harmony-classes.js (right): https://codereview.chromium.org/770333005/diff/20001/src/harmony-classes.js#n... src/harmony-classes.js:21: ['Function.prototype.toMethod', On 2014/12/09 23:39:12, Dmitry Lomov (chromium) wrote: > ...
4 years, 7 months ago (2014-12-10 08:23:31 UTC) #9
Dmitry Lomov (no reviews)
On 2014/12/10 08:23:31, mathias wrote: > https://codereview.chromium.org/770333005/diff/20001/src/harmony-classes.js > File src/harmony-classes.js (right): > > https://codereview.chromium.org/770333005/diff/20001/src/harmony-classes.js#n... > ...
4 years, 7 months ago (2014-12-10 11:11:18 UTC) #10
Dmitry Lomov (no reviews)
On 2014/12/10 11:11:18, Dmitry Lomov (chromium) wrote: > On 2014/12/10 08:23:31, mathias wrote: > > ...
4 years, 7 months ago (2014-12-10 11:25:08 UTC) #11
mathias
On 2014/12/10 11:25:08, Dmitry Lomov (chromium) wrote: > https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg... > > > test/mjsunit/harmony/regexp-flags.js:7: assertEquals('', > ...
4 years, 7 months ago (2014-12-10 12:16:43 UTC) #12
Dmitry Lomov (no reviews)
On 2014/12/10 12:16:43, mathias wrote: > On 2014/12/10 11:25:08, Dmitry Lomov (chromium) wrote: > > ...
4 years, 7 months ago (2014-12-10 12:38:17 UTC) #13
mathias
On 2014/12/10 12:38:17, Dmitry Lomov (chromium) wrote: > On 2014/12/10 12:16:43, mathias wrote: > > ...
4 years, 7 months ago (2014-12-10 13:00:57 UTC) #14
Dmitry Lomov (no reviews)
lgtm % small nit https://codereview.chromium.org/770333005/diff/80001/test/mjsunit/harmony/regexp-flags.js File test/mjsunit/harmony/regexp-flags.js (right): https://codereview.chromium.org/770333005/diff/80001/test/mjsunit/harmony/regexp-flags.js#newcode15 test/mjsunit/harmony/regexp-flags.js:15: // When support for the ...
4 years, 7 months ago (2014-12-10 16:04:44 UTC) #15
Dmitry Lomov (no reviews)
I am sorry I have seconds thoughts :) still lgtm but with couple more nits ...
4 years, 7 months ago (2014-12-10 16:06:29 UTC) #16
mathias
Done. Thanks for the thorough review!
4 years, 7 months ago (2014-12-10 20:06:37 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/770333005/100001
4 years, 7 months ago (2014-12-10 20:13:57 UTC) #19
commit-bot: I haz the power
Committed patchset #6 (id:100001)
4 years, 7 months ago (2014-12-10 20:41:04 UTC) #20
arv (Not doing code reviews)
LGTM I think the tests could have been a bit more extensive but this is ...
4 years, 7 months ago (2014-12-10 21:03:28 UTC) #21
mathias
4 years, 7 months ago (2014-12-13 07:04:28 UTC) #22
Message was sent while issue was closed.
On 2014/12/10 21:03:28, arv wrote:
> I think the tests could have been a bit more extensive but this is good
enough.

What kind of additional tests did you have in mind? I’ll do a follow-up patch
that fixes the configurability soon and I’d love to add some more tests.

Re: configurability, I assumed
https://people.mozilla.org/~jorendorff/es6-draft.html#table-4 applied but
actually the last paragraph of section 17 overrules this:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ecmascript-standard...

Powered by Google App Engine
This is Rietveld 408576698