RegExp: remove last match info override.

RegExp: remove last match info override.

With ES6 21.2.5.8, step 13, we no longer have to keep up the illusion
that matching and calling replace function is interleaved. This is
observable through unspec'ed static properties such as RegExp.$1.

Last match info not working yet.

R=littledan@chromium.org

Committed: https://crrev.com/85e085b7704e4be1329a9a1cf75dff8c6c9157ef
Cr-Commit-Position: refs/heads/master@{#31593}

[Yang, 2015-10-26 10:45:09]

On 2015/10/22 08:55:53, Yang wrote:

Any suggestion on how to move forward on this?

[Dan Ehrenberg, 2015-10-26 17:11:24]

lgtm

[Dan Ehrenberg, 2015-10-26 17:12:01]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2015-10-26 17:12:02]

lgtm

I am still worried this will require a revert, but the API is deprecated (and
the deprecation is well-documented), so I think it's OK for us to create subtle
breakage like this to encourage people to move off of it.

[commit-bot: I haz the power, 2015-10-26 17:12:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418703003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418703003/1

[Dan Ehrenberg, 2015-10-26 17:12:13]

The CQ bit was unchecked by littledan@chromium.org

[Dan Ehrenberg, 2015-10-26 18:22:14]

On 2015/10/26 at 17:12:12, commit-bot wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1418703003/1
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1418703003/1

A couple questions:
- Does this change the behavior of just RegExp.$1, or also the behavior of forms
like "".replace(/foo/, "$1")? I'd imagine the latter form could be more
widespread. At a cursory glance, this seems to be what the failing SunSpider
test is doing.
- Do you know what other browsers do? I'd feel more comfortable with this patch
if we could confirm that there's another browser which doesn't support V8's old
behavior.

[Yang, 2015-10-27 06:39:31]

On 2015/10/26 18:22:14, Dan Ehrenberg wrote:
> On 2015/10/26 at 17:12:12, commit-bot wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1418703003/1
> > View timeline at
> >  https://chromium-cq-status.appspot.com/patch-timeline/1418703003/1
> 
> A couple questions:
> - Does this change the behavior of just RegExp.$1, or also the behavior of
forms
> like "".replace(/foo/, "$1")? I'd imagine the latter form could be more
> widespread. At a cursory glance, this seems to be what the failing SunSpider
> test is doing.
No. This only affects replacer functions that make use of RegExp.$1,
RegExp.lastMatch, etc.

> - Do you know what other browsers do? I'd feel more comfortable with this
patch
> if we could confirm that there's another browser which doesn't support V8's
old
> behavior.
Both FF and Webkit nightly still have the old behavior. Not sure about IE.

[Yang, 2015-10-27 06:43:07]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2015-10-27 06:43:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/1418703003/#ps20001
(title: "rebase")

[commit-bot: I haz the power, 2015-10-27 06:43:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418703003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418703003/20001

[commit-bot: I haz the power, 2015-10-27 06:59:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-27 06:59:01]

Try jobs failed on following builders:
  v8_linux_dbg on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/10059)

[Yang, 2015-10-27 07:57:52]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2015-10-27 07:57:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/1418703003/#ps40001
(title: "fix")

[commit-bot: I haz the power, 2015-10-27 07:58:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418703003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418703003/40001

[Yang, 2015-10-27 07:58:16]

The CQ bit was unchecked by yangguo@chromium.org

[Yang, 2015-10-27 07:59:12]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2015-10-27 07:59:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418703003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418703003/40001

[commit-bot: I haz the power, 2015-10-27 08:24:01]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-27 08:24:13]

Patchset 3 (id:??) landed as
https://crrev.com/85e085b7704e4be1329a9a1cf75dff8c6c9157ef
Cr-Commit-Position: refs/heads/master@{#31593}

[brucedawson, 2015-10-28 17:19:35]

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

[brucedawson, 2015-10-28 17:19:36]

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

https://codereview.chromium.org/1418703003/diff/40001/src/runtime/runtime-reg...
src/runtime/runtime-regexp.cc:1139: Handle<FixedArray> result_array =
builder.array();
I'm worried about this variable declaration. The result_array variable shadows
an identically named function parameter. This isn't dangerous by itself but line
1146 returns result_array. It appears (to my untrained eye) that the intent may
have been to return the variable declared at line 1139, but instead the function
parameter will be returned.

If the code is behaving correctly then I think it is worth renaming the line
1139 result_array to avoid reader confusion - this is one of the more confusing
instances of variable shadowing that I've seen.

[Yang, 2015-10-29 12:55:28]

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

https://codereview.chromium.org/1418703003/diff/40001/src/runtime/runtime-reg...
src/runtime/runtime-regexp.cc:1139: Handle<FixedArray> result_array =
builder.array();
On 2015/10/28 17:19:36, brucedawson wrote:
> I'm worried about this variable declaration. The result_array variable shadows
> an identically named function parameter. This isn't dangerous by itself but
line
> 1146 returns result_array. It appears (to my untrained eye) that the intent
may
> have been to return the variable declared at line 1139, but instead the
function
> parameter will be returned.
> 
> If the code is behaving correctly then I think it is worth renaming the line
> 1139 result_array to avoid reader confusion - this is one of the more
confusing
> instances of variable shadowing that I've seen.

Thanks. See https://codereview.chromium.org/1407213005

[brucedawson, 2015-10-29 16:54:08]

Thanks for the clarifying change.