Bug 5409 - slice() testcase doesn't pass
Summary: slice() testcase doesn't pass
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Other Linux
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-10-17 23:23 PDT by George Staikos
Modified: 2005-11-05 21:55 PST (History)
0 users

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description George Staikos 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.
Comment 1 George Staikos 2005-10-17 23:24:17 PDT
Created attachment 4394 [details]
patch to fix slice() in KJS
Comment 2 Maciej Stachowiak 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.
Comment 3 Darin Adler 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.
Comment 4 Geoffrey Garen 2005-10-24 19:42:41 PDT
Filed a new bug to track using toInteger: 5490.
Comment 5 Geoffrey Garen 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.
Comment 6 Darin Adler 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.
Comment 7 Geoffrey Garen 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.