Closed Bug 421325 Opened 14 years ago Closed 14 years ago

array_join_sub does not handle holes in dense-arrays correctly

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9beta5

People

(Reporter: crowderbt, Assigned: crowderbt)

References

Details

Attachments

(1 file, 2 obsolete files)

This should become a test-case:

js> Array.prototype[1] = 'bar'           
bar
js> a = []; a[0]='foo'; a[2] = 'baz'; a

Expected:
foo,bar,baz

Currently:
foo,,baz

Patch coming.
Attached patch proposed fix :( (obsolete) — Splinter Review
As discussed in IM, perhaps there is a win to be had by keeping the current "hole" behavior, if obj.prototype == Array.prototype and Array.prototype.length == 0?  I'll toy with that in a second, just want to capture this patch.
Assignee: general → crowder
Status: NEW → ASSIGNED
Attachment #307767 - Flags: review?(shaver)
Comment on attachment 307767 [details] [diff] [review]
proposed fix :(

Why not only fall through to the slower path if we see a hole?

if (OBJ_IS_DENSE() && index < DENSE_LENGTH && (*vp = obj->dslots[index]) != JSVAL_HOLE) { *hole = JS_FALSE; return JS_TRUE; }

?
Attachment #307767 - Flags: review?(shaver) → review-
Attached patch less unhappy (obsolete) — Splinter Review
Yeah, this is better.  Will test shortly.
Attachment #307767 - Attachment is obsolete: true
Attachment #307776 - Flags: review?(shaver)
Comment on attachment 307776 [details] [diff] [review]
less unhappy

r/a=shaver on the jsarray part, please add a test as well for this case; your snippets from IM will do fine.

The js.c part isn't for this bug, and looks unfinished regardless, so don't check that part in. :)
Attachment #307776 - Flags: review?(shaver)
Attachment #307776 - Flags: review+
Attachment #307776 - Flags: approval1.9+
Attachment #307776 - Attachment is obsolete: true
jsarray.c: 3.167
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Checking in regress-421325.js;
/cvsroot/mozilla/js/tests/ecma_3/Array/regress-421325.js,v  <--  regress-421325.js
initial revision: 1.1
Flags: in-testsuite+
Flags: in-litmus-
Depends on: 421960
v 1.9.0
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.