Bug 9234 - Implement $&, $' and $` replacement codes in String.prototype.replace
Summary: Implement $&, $' and $` replacement codes in String.prototype.replace
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 7919 (view as bug list)
Depends on:
Blocks: 9205
  Show dependency treegraph
 
Reported: 2006-06-02 08:42 PDT by mitz
Modified: 2006-12-19 10:10 PST (History)
2 users (show)

See Also:


Attachments
Patch without test case and change log (2.31 KB, patch)
2006-06-02 08:45 PDT, mitz
no flags Details | Formatted Diff | Diff
Patch including change log and test (7.28 KB, patch)
2006-06-02 14:48 PDT, mitz
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2006-06-02 08:42:14 PDT
According to Ecma-262 15.5.4.11, if replaceValue is a string, then $& should be replaced with the matched substring, $` with everything preceding the matched substring, and $' with everything following the matched substring. For example,

"abcdefg".replace(/d(e)/, "[$&]") == "abc[de]fg" (currently achievable using $0)
"abcdefg".replace(/d(e)/, "[$`]") == "abc[abc]fg"
"abcdefg".replace(/d(e)/, "[$']") == "abc[fg]fg"
Comment 1 mitz 2006-06-02 08:45:45 PDT
Created attachment 8665 [details]
Patch without test case and change log

This passes all JavaScriptCore tests and WebCore layout test, but I didn't know what kind of test to attach to it. Does it need to be a JSC test?
Comment 2 Geoffrey Garen 2006-06-02 11:28:53 PDT
Comment on attachment 8665 [details]
Patch without test case and change log

for (int i = 0; (i = substitutedReplacement.find(UString("$"), i)) != -1; i++) {

This line confused the dickens out of me, because 'i = 0' has no meaning, and 'i++' has meaning only as a side-effect in the test that follows it. I know it's not something you changed, but consider one of these (maybe my perception is skewed):

int i = -1;
while ((i = substitutedReplacement.find(UString("$"), i + 1)) != -1)

OR

for (int i = substitutedReplacement.find(UString("$")); i != -1; i = substitutedReplacement.find(UString("$"), i + 1))

This comment would change, too:
      i += backrefLength - 1; // -1 offsets i++ (--> 'i + 1')


Anyway, looks good. We discussed how to write a layout test, so I'm going to clear the review bit for now.
Comment 3 mitz 2006-06-02 14:48:54 PDT
Created attachment 8671 [details]
Patch including change log and test

This patch also adds double-digit back references ($nn) as required by the spec.
Comment 4 David Kilzer (:ddkilzer) 2006-06-02 21:52:38 PDT
Committed revision 14705.
Comment 5 mitz 2006-12-19 10:10:43 PST
*** Bug 7919 has been marked as a duplicate of this bug. ***