Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years ago by mathiasb Modified:
6 years ago Reviewers:
Dmitry Lomov (no reviews), arv (Not doing code reviews), mathias CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Project:
v8 Visibility:
Public. |
DescriptionImplement 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 : #
Created: 6 years ago
Messages
Total messages: 22 (3 generated)
mathias@qiwi.be changed reviewers: + arv@chromium.org, mathias@qiwi.be
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. Am I doing something crazy here?
dslomov@chromium.org changed reviewers: + dslomov@chromium.org
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 bootstrapping code (code that executes on loading natives). [other uses in this file are ok since they are inside function bodies] What we typically do is create a separate file, harmony-regexp.js, that is loaded conditionally under FLAG_harmony_regexp.
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): > > 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 bootstrapping code (code that executes on > loading natives). > [other uses in this file are ok since they are inside function bodies] > > What we typically do is create a separate file, harmony-regexp.js, that is > loaded conditionally under FLAG_harmony_regexp. Thanks for the pointers, Dmitry and Erik (IRC)! Patch updated.
https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg... File test/mjsunit/harmony/regexp-flags.js (right): https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg... test/mjsunit/harmony/regexp-flags.js:27: .get.call(object); Nit: +2 indent Will fix in the next patch, along with any feedback.
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', Looks like this is from some other patch? 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#ne... src/harmony-regexp.js:17: throw MakeTypeError('home_object_non_object', Wrong error message. https://codereview.chromium.org/770333005/diff/20001/src/harmony-regexp.js#ne... src/harmony-regexp.js:30: %DefineAccessorPropertyUnchecked($RegExp.prototype, 'flags', RegExpGetFlags, You should pass 'undefined' here instead of NoOpSetter. Regexp.prototype.flags do not have a setter. https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg... File test/mjsunit/harmony/regexp-flags.js (right): https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg... test/mjsunit/harmony/regexp-flags.js:5: // Flags: --harmony-regexps Also add test that validates properties of Regexp.prototype.flag from its property descriptor (has getter, has no setter)
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#ne... src/harmony-regexp.js:15: var RegExpGetFlags = function() { Move this out of ExtendRegExpPrototype. https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg... File test/mjsunit/harmony/regexp-flags.js (right): https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg... test/mjsunit/harmony/regexp-flags.js:7: assertEquals('', RegExp.prototype.flags); Shouldn't this throw since the prototype object does not have [[OriginalFlags]]? I guess this is a really a bug in our implementation of RegExp.prototype.global. But once/if we fix that then this test will start to fail. https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg... test/mjsunit/harmony/regexp-flags.js:8: RegExp.prototype.flags = 'foo'; A better test is to get the descriptor and validate it.
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: > Looks like this is from some other patch? It was intentional; I wanted to re-use the same error message. Updated the patch now to use a new, separate error message. https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg... File test/mjsunit/harmony/regexp-flags.js (right): https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg... test/mjsunit/harmony/regexp-flags.js:7: assertEquals('', RegExp.prototype.flags); On 2014/12/10 02:54:50, arv wrote: > Shouldn't this throw since the prototype object does not have [[OriginalFlags]]? > > I guess this is a really a bug in our implementation of RegExp.prototype.global. > But once/if we fix that then this test will start to fail. Ack. What do you want to do here – fix the separate bug later and leave the test for now? Or just remove this test?
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... > src/harmony-classes.js:21: ['Function.prototype.toMethod', > On 2014/12/09 23:39:12, Dmitry Lomov (chromium) wrote: > > Looks like this is from some other patch? > > It was intentional; I wanted to re-use the same error message. Updated the patch > now to use a new, separate error message. > > https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg... > File test/mjsunit/harmony/regexp-flags.js (right): > > https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg... > test/mjsunit/harmony/regexp-flags.js:7: assertEquals('', > RegExp.prototype.flags); > On 2014/12/10 02:54:50, arv wrote: > > Shouldn't this throw since the prototype object does not have > [[OriginalFlags]]? > > > > I guess this is a really a bug in our implementation of > RegExp.prototype.global. > > But once/if we fix that then this test will start to fail. > > Ack. What do you want to do here – fix the separate bug later and leave the test > for now? Or just remove this test? Remove the test. It is incorrect per spec. What you really want to test here is subclasses of RegExp but we do not support that currently.
On 2014/12/10 11:11:18, Dmitry Lomov (chromium) wrote: > 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... > > src/harmony-classes.js:21: ['Function.prototype.toMethod', > > On 2014/12/09 23:39:12, Dmitry Lomov (chromium) wrote: > > > Looks like this is from some other patch? > > > > It was intentional; I wanted to re-use the same error message. Updated the > patch > > now to use a new, separate error message. > > > > > https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg... > > File test/mjsunit/harmony/regexp-flags.js (right): > > > > > https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg... > > test/mjsunit/harmony/regexp-flags.js:7: assertEquals('', > > RegExp.prototype.flags); > > On 2014/12/10 02:54:50, arv wrote: > > > Shouldn't this throw since the prototype object does not have > > [[OriginalFlags]]? > > > > > > I guess this is a really a bug in our implementation of > > RegExp.prototype.global. > > > But once/if we fix that then this test will start to fail. > > > > Ack. What do you want to do here – fix the separate bug later and leave the > test > > for now? Or just remove this test? > > Remove the test. It is incorrect per spec. > What you really want to test here is subclasses of RegExp but we do not support > that currently. [Sorry that came out unclear: - the test asserts the behavior that is contrary to the spec, so should be removed - subclassing comment is separate, not related to assertEquals('', RegExp.prototype.flags line]
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('', > > > RegExp.prototype.flags); > > > On 2014/12/10 02:54:50, arv wrote: > > > > Shouldn't this throw since the prototype object does not have > > > [[OriginalFlags]]? > > > > > > > > I guess this is a really a bug in our implementation of > > > RegExp.prototype.global. > > > > But once/if we fix that then this test will start to fail. > > > > > > Ack. What do you want to do here – fix the separate bug later and leave the > > test > > > for now? Or just remove this test? > > > > Remove the test. It is incorrect per spec. > > What you really want to test here is subclasses of RegExp but we do not > support > > that currently. > > [Sorry that came out unclear: > - the test asserts the behavior that is contrary to the spec, so should be > removed > - subclassing comment is separate, not related to assertEquals('', > RegExp.prototype.flags line] I’m confused. The spec for `RegExp.prototype.global` says: > 3. If R does not have an [[OriginalFlags]] internal slot throw a TypeError exception. But `RegExp.prototype.flags` doesn’t seem to have such a restriction. What makes you say this should throw?
On 2014/12/10 12:16:43, mathias wrote: > 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('', > > > > RegExp.prototype.flags); > > > > On 2014/12/10 02:54:50, arv wrote: > > > > > Shouldn't this throw since the prototype object does not have > > > > [[OriginalFlags]]? > > > > > > > > > > I guess this is a really a bug in our implementation of > > > > RegExp.prototype.global. > > > > > But once/if we fix that then this test will start to fail. > > > > > > > > Ack. What do you want to do here – fix the separate bug later and leave > the > > > test > > > > for now? Or just remove this test? > > > > > > Remove the test. It is incorrect per spec. > > > What you really want to test here is subclasses of RegExp but we do not > > support > > > that currently. > > > > [Sorry that came out unclear: > > - the test asserts the behavior that is contrary to the spec, so should be > > removed > > - subclassing comment is separate, not related to assertEquals('', > > RegExp.prototype.flags line] > > I’m confused. The spec for `RegExp.prototype.global` says: > > > 3. If R does not have an [[OriginalFlags]] internal slot throw a TypeError > exception. > > But `RegExp.prototype.flags` doesn’t seem to have such a restriction. What makes > you say this should throw? `flags` call `this.global` and that throws since `this` in this case is RegExp.prototype.
On 2014/12/10 12:38:17, Dmitry Lomov (chromium) wrote: > On 2014/12/10 12:16:43, mathias wrote: > > I’m confused. The spec for `RegExp.prototype.global` says: > > > > > 3. If R does not have an [[OriginalFlags]] internal slot throw a TypeError > > exception. > > > > But `RegExp.prototype.flags` doesn’t seem to have such a restriction. What > makes > > you say this should throw? > > `flags` call `this.global` and that throws since `this` in this case is > RegExp.prototype. Ah, of course! Test removed. Since none of the major ECMAScript engines seem to implement step 3 of `RegExp.prototype.global`’s algorithm (and similar for the other `RegExp.prototype` getters) maybe the spec should just be changed to reflect reality. I’ve filed https://bugs.ecmascript.org/show_bug.cgi?id=3432 for that.
lgtm % small nit https://codereview.chromium.org/770333005/diff/80001/test/mjsunit/harmony/reg... File test/mjsunit/harmony/regexp-flags.js (right): https://codereview.chromium.org/770333005/diff/80001/test/mjsunit/harmony/reg... test/mjsunit/harmony/regexp-flags.js:15: // When support for the `u` flag is added, uncomment the first line below and Please add a TODO here. Feel free to put me as an owner of this TODO (TODO(dslomov)
I am sorry I have seconds thoughts :) still lgtm but with couple more nits - sorry. https://codereview.chromium.org/770333005/diff/80001/src/harmony-regexp.js File src/harmony-regexp.js (right): https://codereview.chromium.org/770333005/diff/80001/src/harmony-regexp.js#ne... src/harmony-regexp.js:4: style nit: 'use strict' https://codereview.chromium.org/770333005/diff/80001/src/harmony-regexp.js#ne... src/harmony-regexp.js:11: var RegExpGetFlags = function() { style nit: just use function RegExpGetFlags here.
Done. Thanks for the thorough review!
The CQ bit was checked by dslomov@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/770333005/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
LGTM I think the tests could have been a bit more extensive but this is good enough.
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... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
