Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(173)

Issue 2685183003: [regexp] Sticky handling in fast slow path (Closed)

Created:
3 years, 10 months ago by jgruber
Modified:
3 years, 10 months ago
Reviewers:
Dan Ehrenberg, Yang
CC:
v8-reviews_googlegroups.com
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[regexp] Correct lastIndex behavior in RegExp.prototype[@@replace] @@replace has a pretty complex implementation, taking different paths for various situations (e.g.: global/nonglobal regexp, functional/string replace argument, etc.). Each of these paths must implement similar logic for calling into the RegExpBuiltinExec spec operation, and many paths get this subtly wrong. This CL fixes a couple of issues related to the way @@replace handles lastIndex: * All paths now respect lastIndex when calling into exec (some used to assume 0). * lastIndex is now advanced after a successful match for sticky regexps. * lastIndex is now only reset to 0 on failure for sticky regexps. BUG=v8:5361 Review-Url: https://codereview.chromium.org/2685183003 Cr-Commit-Position: refs/heads/master@{#43234} Committed: https://chromium.googlesource.com/v8/v8/+/c0fe56e63debf06b420ec34272b697c041056284

Patch Set 1 #

Patch Set 2 : Fix simple string fast path #

Patch Set 3 : Fix DCHECK #

Patch Set 4 : Reset lastIndex on failure iff sticky #

Patch Set 5 : Test expectations #

Patch Set 6 : Remove unused vars #

Patch Set 7 : ToUint32 and push loads to uses #

Patch Set 8 : Revert "Remove custom ToUint32" #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+88 lines, -45 lines) Patch
M src/builtins/builtins-regexp.cc View 7 chunks +30 lines, -20 lines 2 comments Download
M src/runtime/runtime-regexp.cc View 6 chunks +41 lines, -9 lines 0 comments Download
M test/mjsunit/regress/regress-2437.js View 4 chunks +17 lines, -7 lines 0 comments Download
M test/mozilla/mozilla.status View 2 chunks +0 lines, -5 lines 0 comments Download
M test/test262/test262.status View 1 chunk +0 lines, -4 lines 0 comments Download

Messages

Total messages: 40 (36 generated)
jgruber
This fixes a couple of issues around lastIndex handling for sticky regexps. Please take a ...
3 years, 10 months ago (2017-02-13 12:15:38 UTC) #34
Yang
On 2017/02/13 12:15:38, jgruber wrote: > This fixes a couple of issues around lastIndex handling ...
3 years, 10 months ago (2017-02-15 12:24:08 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2685183003/140001
3 years, 10 months ago (2017-02-16 08:49:32 UTC) #37
commit-bot: I haz the power
3 years, 10 months ago (2017-02-16 09:21:44 UTC) #40
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/c0fe56e63debf06b420ec34272b697c0410...

Powered by Google App Engine
This is Rietveld 408576698