Closed
Bug 1343375
Opened 7 years ago
Closed 7 years ago
RegExp.prototype[@@replace] needs to re-read global/sticky flags for non-global opt
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
References
Details
Attachments
(1 file, 1 obsolete file)
13.33 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
Test case: --- var r = /a/y; r.lastIndex = { valueOf() { r.compile("a", "") return 0; } }; r[Symbol.replace]("a"); print(r.lastIndex); --- Expected: Prints 0 Actual: Prints 1
Assignee | ||
Comment 1•7 years ago
|
||
This issue also affects RegExp.prototype[@@match]: --- var r = /a/y; r.lastIndex = { valueOf() { r.compile("a", "") return 0; } }; r[Symbol.match]("a"); print(r.lastIndex); --- Expected: Prints 0 Actual: Prints 1
Assignee | ||
Comment 2•7 years ago
|
||
I think it makes sense this fix this bug before working on bug 1317397, therefore marking this one as blocking 1317397.
Blocks: 1317397
Assignee | ||
Comment 3•7 years ago
|
||
Also fixes bug 1343369 because it's related to this issue. Updating RegExpLocalMatchOpt to properly handle recompilations made RegExpLocalMatchOpt identical to RegExpBuiltinExec, therefore I've simply removed RegExpLocalMatchOpt and called RegExpBuiltinExec instead.
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8842848 -
Flags: review?(arai.unmht)
Comment 4•7 years ago
|
||
Comment on attachment 8842848 [details] [diff] [review] bug1343375.patch Review of attachment 8842848 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Interestingly I don't see any notable performance difference between using RegExpLocalMatchOpt and RegExpBuiltinExec now. this is a nice code reduction :) ::: js/src/builtin/RegExp.js @@ +307,1 @@ > if (functionalReplace) { please remove braces @@ +309,2 @@ > } > if (firstDollarIndex !== -1) { here too ::: js/src/builtin/RegExpLocalReplaceOpt.h.js @@ +21,5 @@ > #endif > ) > { > + // 21.2.5.2.2 RegExpBuiltinExec, step 4. > + var lastIndex = ToLength(rx.lastIndex); it would be nice to explain this operation can recompile RegExp, and that's the reason why steps 6-7 below checks global too, even if this function is optimized for local replace.
Attachment #8842848 -
Flags: review?(arai.unmht) → review+
Assignee | ||
Comment 6•7 years ago
|
||
Updated patch per review comments, carrying r+ from arai.
Attachment #8842848 -
Attachment is obsolete: true
Attachment #8842864 -
Flags: review+
Assignee | ||
Comment 7•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6e3f7a1751ee69c0ac1086d36486afbf6bce075
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a9c7e13a967c Update RegExp.prototype.replace and .match to call ToLength(lastIndex) for non-global RegExp and handle recompilations. r=arai
Keywords: checkin-needed
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a9c7e13a967c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•