Bug 5409

Summary:

slice() testcase doesn't pass

Product:

WebKit

Reporter:

George Staikos <staikos>

Component:

JavaScriptCore

Assignee:

Geoffrey Garen <ggaren>

Status:

RESOLVED FIXED

Severity:

Normal

Priority:

Version:

420+

Hardware:

Other

OS:

Linux

Attachments:

Description	Flags
patch to fix slice() in KJS	none
Revised Patch	darin: review+

George Staikos

Reported 2005-10-17 23:23:49 PDT

String slice() testcases are not passing. attached KJS patch fixes it. It should be fairly easy to merge by hand.

Attachments
patch to fix slice() in KJS (1.20 KB, patch) 2005-10-17 23:24 PDT, George Staikos	no flags	Details Formatted Diff Diff
Revised Patch (1.92 KB, patch) 2005-10-24 20:13 PDT, Geoffrey Garen	darin: review+	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

George Staikos

Comment 1 2005-10-17 23:24:17 PDT

Created attachment 4394 [details] patch to fix slice() in KJS

Maciej Stachowiak

Comment 2 2005-10-17 23:31:53 PDT

Comment on attachment 4394 [details] patch to fix slice() in KJS George and I discussed how it would be safer against extreme values ot use toInteger and leave things as double until after the range check, but I think it's still an improvement to merge this fix from KJS.

Darin Adler

Comment 3 2005-10-18 09:55:30 PDT

Comment on attachment 4394 [details] patch to fix slice() in KJS The standard is clear on this point: toUInt32 is incorrect, toInteger is correct. But that's a bug that can be fixed separately.

Geoffrey Garen

Comment 4 2005-10-24 19:42:41 PDT

Filed a new bug to track using toInteger: 5490.

Geoffrey Garen

Comment 5 2005-10-24 20:13:14 PDT

Created attachment 4466 [details] Revised Patch Psych. Decided to fix it myself here. Also fixed the end == undefined case.

Darin Adler

Comment 6 2005-10-25 08:16:52 PDT

Comment on attachment 4466 [details] Revised Patch There are a few examples here of braces for single lines. That doesn't match our coding style. Also, I prefer just doing a return to setting the value of String. Finally, I'd like test cases for what you fixed here. But it looks good to me, so review+ anyway.

Geoffrey Garen

Comment 7 2005-11-05 21:55:32 PST

I removed the single braces and added a layout test for the cases not covered by the mozilla test suite before landing. I felt iffy about returning immediately rather than setting result, though. Setting result is the idiom for StringProtoFuncImp::callAsFunction. If we don't like that idiom, I think we should file a separate bug and change all cases of it in the function, rather than creating an idiosyncracy for this one case.

Note You need to log in before you can comment on or make changes to this bug.