RegExp: Add support for the ES6-proposed sticky flag

RegExp: Add support for the ES6-proposed sticky flag

R=yangguo@chromium.org, rossberg@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=24031

[Erik Corry, 2014-09-15 08:54:33]

The 'y' flag has been supported by Firefox for a long time. It makes a regexp
that does not have the implicit ".*" at the start, ie it does not search
forwards for a match, but tries to match at the starting position or not at all.
 It's a proposed part of ES6.

For now, the feature is behind the --harmony-regexps flag, which is activated
together with the other --harmony features.

[Erik Corry, 2014-09-15 09:05:58]

On 2014/09/15 08:54:33, Erik Corry wrote:
> The 'y' flag has been supported by Firefox for a long time. It makes a regexp
> that does not have the implicit ".*" at the start, ie it does not search
> forwards for a match, but tries to match at the starting position or not at
all.
>  It's a proposed part of ES6.
> 
> For now, the feature is behind the --harmony-regexps flag, which is activated
> together with the other --harmony features.

The test files pass on Firefox.

[Yang, 2014-09-15 09:23:16]

Looking good. I got some suggestions. And also what seems to be missing is
updating .lastIndex on match success for sticky regexps.

https://codereview.chromium.org/567313003/diff/20001/src/jsregexp.cc
File src/jsregexp.cc (right):

https://codereview.chromium.org/567313003/diff/20001/src/jsregexp.cc#newcode6064
src/jsregexp.cc:6064: // this expression is anchored at the beginning.
... or sticky.

https://codereview.chromium.org/567313003/diff/20001/src/regexp.js
File src/regexp.js (right):

https://codereview.chromium.org/567313003/diff/20001/src/regexp.js#newcode25
src/regexp.js:25: if (%HarmonyRegExps())
While I'm fine with this (will be removed eventually anyways), I think adding a
global value to the builtins object during bootstrapping to mirror the flag is
cleaner.

https://codereview.chromium.org/567313003/diff/20001/src/regexp.js#newcode239
src/regexp.js:239: if (!this.sticky &&
We also need to record .lastIndex for sticky. If I'm not mistaken, the global
and sticky case in here is the same. So we could just change line 219 to

if (this.global || this.sticky) {

https://codereview.chromium.org/567313003/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/567313003/diff/20001/src/runtime.cc#newcode1265
src/runtime.cc:1265: if (FLAG_harmony_regexps)
You could also use isolate->heap()->ToBoolean(FLAG_harmony_regexps)

https://codereview.chromium.org/567313003/diff/20001/test/mjsunit/harmony/reg...
File test/mjsunit/harmony/regexp-sticky.js (right):

https://codereview.chromium.org/567313003/diff/20001/test/mjsunit/harmony/reg...
test/mjsunit/harmony/regexp-sticky.js:48:
assertFalse(!!"..foobar".match(stickyplain));
According to 21.2.5.2.2, 19, we should set .lastIndex when the regexp is sticky
or global. This is not tested anywhere here.

[mathias, 2014-09-15 15:19:09]

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

[mathias, 2014-09-15 15:19:10]

https://codereview.chromium.org/567313003/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/567313003/diff/20001/src/runtime.cc#newcode2600
src/runtime.cc:2600: // space for the 'sticky' flag, since is from the snapshot,
but must work both
Typo: “since is”

[Erik Corry, 2014-09-16 17:49:40]

https://codereview.chromium.org/567313003/diff/20001/src/jsregexp.cc
File src/jsregexp.cc (right):

https://codereview.chromium.org/567313003/diff/20001/src/jsregexp.cc#newcode6064
src/jsregexp.cc:6064: // this expression is anchored at the beginning.
On 2014/09/15 09:23:16, Yang wrote:
> ... or sticky.

Done.

https://codereview.chromium.org/567313003/diff/20001/src/regexp.js
File src/regexp.js (right):

https://codereview.chromium.org/567313003/diff/20001/src/regexp.js#newcode25
src/regexp.js:25: if (%HarmonyRegExps())
On 2014/09/15 09:23:16, Yang wrote:
> While I'm fine with this (will be removed eventually anyways), I think adding
a
> global value to the builtins object during bootstrapping to mirror the flag is
> cleaner.

Done.

https://codereview.chromium.org/567313003/diff/20001/src/regexp.js#newcode239
src/regexp.js:239: if (!this.sticky &&
On 2014/09/15 09:23:16, Yang wrote:
> We also need to record .lastIndex for sticky. If I'm not mistaken, the global
> and sticky case in here is the same. So we could just change line 219 to
> 
> if (this.global || this.sticky) {

Done, and also added string length check.

https://codereview.chromium.org/567313003/diff/20001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/567313003/diff/20001/src/runtime.cc#newcode1265
src/runtime.cc:1265: if (FLAG_harmony_regexps)
On 2014/09/15 09:23:16, Yang wrote:
> You could also use isolate->heap()->ToBoolean(FLAG_harmony_regexps)

This is gone now.

https://codereview.chromium.org/567313003/diff/20001/src/runtime.cc#newcode2600
src/runtime.cc:2600: // space for the 'sticky' flag, since is from the snapshot,
but must work both
On 2014/09/15 15:19:10, mathias wrote:
> Typo: “since is”

Done.

https://codereview.chromium.org/567313003/diff/20001/test/mjsunit/harmony/reg...
File test/mjsunit/harmony/regexp-sticky.js (right):

https://codereview.chromium.org/567313003/diff/20001/test/mjsunit/harmony/reg...
test/mjsunit/harmony/regexp-sticky.js:48:
assertFalse(!!"..foobar".match(stickyplain));
On 2014/09/15 09:23:16, Yang wrote:
> According to 21.2.5.2.2, 19, we should set .lastIndex when the regexp is
sticky
> or global. This is not tested anywhere here.

Done.

[Yang, 2014-09-17 07:16:15]

LGT with one comment.

https://codereview.chromium.org/567313003/diff/100001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/567313003/diff/100001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1423: v8::False(external_isolate);
Let's not unnecessarily use API handles. We can just have 

    Runtime::DefineObjectProperty(
        builtins,
       
factory()->InternalizeOneByteString(STATIC_CHAR_VECTOR("harmony_regexps")),
        isolate()->heap()->ToBoolean(FLAG_harmony_regexps), NONE).Assert();

I assume you took the code from Genesis::Genesis. There is no internal way to
initialize a typed array, which is why API methods are used there.

[Yang, 2014-09-17 07:16:40]

On 2014/09/17 07:16:15, Yang wrote:
> LGT with one comment.
> 
> https://codereview.chromium.org/567313003/diff/100001/src/bootstrapper.cc
> File src/bootstrapper.cc (right):
> 
>
https://codereview.chromium.org/567313003/diff/100001/src/bootstrapper.cc#new...
> src/bootstrapper.cc:1423: v8::False(external_isolate);
> Let's not unnecessarily use API handles. We can just have 
> 
>     Runtime::DefineObjectProperty(
>         builtins,
>        
> factory()->InternalizeOneByteString(STATIC_CHAR_VECTOR("harmony_regexps")),
>         isolate()->heap()->ToBoolean(FLAG_harmony_regexps), NONE).Assert();
> 
> I assume you took the code from Genesis::Genesis. There is no internal way to
> initialize a typed array, which is why API methods are used there.

...M. I meant to write LGTM (with a comment).

[Erik Corry, 2014-09-17 17:06:19]

https://codereview.chromium.org/567313003/diff/100001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/567313003/diff/100001/src/bootstrapper.cc#new...
src/bootstrapper.cc:1423: v8::False(external_isolate);
On 2014/09/17 07:16:14, Yang wrote:
> Let's not unnecessarily use API handles. We can just have 
> 
>     Runtime::DefineObjectProperty(
>         builtins,
>        
> factory()->InternalizeOneByteString(STATIC_CHAR_VECTOR("harmony_regexps")),
>         isolate()->heap()->ToBoolean(FLAG_harmony_regexps), NONE).Assert();
> 
> I assume you took the code from Genesis::Genesis. There is no internal way to
> initialize a typed array, which is why API methods are used there.

Done.

[Erik Corry, 2014-09-18 11:34:50]

Committed patchset #8 (id:140001) manually as 24031 (presubmit successful).

[marja, 2014-12-18 14:01:19]

Post-commit comment: RegExpMirror seems to contain the other flags, why is the
sticky flag not added there?

[Erik Corry, 2014-12-18 14:19:37]

On 2014/12/18 14:01:19, marja (ooo until Jan 7th) wrote:
> Post-commit comment: RegExpMirror seems to contain the other flags, why is the
> sticky flag not added there?

It's a bug, I think.