Bug 16787 - array.splice() with 1 argument not working
Summary: array.splice() with 1 argument not working
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2008-01-08 12:37 PST by Jan Boeijink
Modified: 2008-01-14 01:14 PST (History)
6 users (show)

See Also:


Attachments
array.splice() with one element (216 bytes, text/html)
2008-01-08 12:40 PST, Jan Boeijink
no flags Details
proposed fix (5.84 KB, patch)
2008-01-09 05:57 PST, Alexey Proskuryakov
ddkilzer: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Boeijink 2008-01-08 12:37:48 PST
splice() with 1 element is not working in Webkit. Compare behaviour with Firefox.
array.splice(index) should remove all items starting from index. 
See attachment.
Comment 1 Jan Boeijink 2008-01-08 12:40:30 PST
Created attachment 18332 [details]
array.splice() with one element
Comment 2 David Kilzer (:ddkilzer) 2008-01-08 21:32:51 PST
Same issue occurs with Safari 2.0.4 (419.3) and Safari 3.0.4 (523.12.2) with original WebKit(s) on Mac OS  X 10.4.11 (8S165).

Comment 3 Alexey Proskuryakov 2008-01-09 05:10:29 PST
According to <http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Objects:Array:splice>, this is a non-standard Mozilla extension (not to imply that we don't want to support it).
Comment 4 David Kilzer (:ddkilzer) 2008-01-09 05:50:46 PST
Safari 3.0.4, MSIE 7 and Opera 9.24 follow the standard, and thus splice() with one argument does nothing in these browsers.

Comment 5 Alexey Proskuryakov 2008-01-09 05:57:33 PST
Created attachment 18350 [details]
proposed fix

I have a fix anyway, a reviewer can decide whether we want this.
Comment 6 David Kilzer (:ddkilzer) 2008-01-09 06:05:04 PST
Comment on attachment 18350 [details]
proposed fix

r=me

Does this change impact SunSpider performance?  The following two tests use splice:

SunSpider/tests/date-format-tofte.js
SunSpider/tests/string-unpack-code.js
Comment 7 Darin Adler 2008-01-13 11:17:02 PST
Comment on attachment 18350 [details]
proposed fix

+    if (!args.size())

+    if (args.size() > 1)

Our usual design is to always consider undefined arguments rather than checking number of arguments. So rather than checking for lack of arguments, we detect the case where arguments are undefined, treating a smaller number of arguments identically to a call with explicit "undefined" for th extra arguments.

I'd like to see test cases that pass undefined to make this clear one way or another. Normally I consider it forbidden to actually look at args.size(), but if we're trying to match the Mozilla behavior it's possible it's needed here.
Comment 8 Alexey Proskuryakov 2008-01-13 11:23:53 PST
(In reply to comment #7)
The included test checks for this, if I understood you correctly:

+shouldBe("arr.splice()", "undefined")
+shouldBe("arr", "['a','b','c','d']");
+shouldBe("arr.splice(undefined)", "['a','b','c','d']")
+shouldBe("arr", "[]");

(similar for the second argument). Indeed, I was trying to match Mozilla behavior.

If anyone wonders why I haven't landed this yet - it's because I didn't have the time to build release and test performance.
Comment 9 Jan Boeijink 2008-01-13 13:05:56 PST
This may be off-topic, but Darin's comment reminded me that before I found this little omission, I wondered if this could be possible:

array1 = ['a','b','c','d'];
array2 = array1.splice(2,undefined,'e','f','g');

--> array1 = ['a','b','e','f','g'];
--> array2 = ['c','d'];

In other words, remove unknown amount of elements starting from position 2 like in splice(2), and add some elements. Ofcourse there are other ways...

The actual result (in Webkit and Firefox) is:
 --> array1 = ['a','b','e','f','g','c','d'];
Comment 10 Alexey Proskuryakov 2008-01-14 01:14:40 PST
Committed revision 29459.