Closed Bug 543682 Opened 13 years ago Closed 13 years ago

don't let holes escape dense arrays via typed arrays

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: vlad)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

copyFrom() in typed arrays wasn't handling JSVAL_HOLE.  Handle it as 0.  Also handle the case when an array's capacity might be less than its length by just taking the slower path.
Attachment #424747 - Flags: review?(brendan)
Comment on attachment 424747 [details] [diff] [review]
handle holes and length > capacity

Needs a test, for sure.  Is 0 the right value?  I would have expected that hole -> double would become a NaN.
Attachment #424747 - Flags: review?(brendan)
Attached patch v2Splinter Review
V2, now with a test, and a TODO.  Spec's still in flux as to what the behaviour should be (it'll likely be NaN for float arrays), but I want to wait for that to settle before implementing that -- it'll be a bit of code rework as this function is currently not dependent on the array type.

I'd like to get this in soon though, since it fixes a crash (JSVAL_HOLE looks like an object further down the pipe and results in a boom).
Attachment #424747 - Attachment is obsolete: true
Attachment #425105 - Flags: review?(brendan)
Comment on attachment 425105 [details] [diff] [review]
v2

crash fix -- bouncing this over to jorendorff
Attachment #425105 - Flags: review?(brendan) → review?(jorendorff)
Thanks to jorendorff, feel free to bounce to him -- he's designer of the ctypes stuff so kind of native-type reflection czar ;-).

One thing i noticed skimming the patch: the tests that check a[0] == NaN can't ever pass, since NaN != NaN (yes, irreflexive -- dig it, it's IEEE754). To test whether x is a NaN, test (x != x).

/be
Argh, I keep forgetting about that.  Might be a bug in my test harness, as those tests are passing locally -- looking into it, thanks!
(In reply to comment #4)
> To test whether x is a NaN, test (x != x).

...or use isNaN(), right? Seems clearer to me.
(In reply to comment #6)
> (In reply to comment #4)
> > To test whether x is a NaN, test (x != x).
> 
> ...or use isNaN(), right? Seems clearer to me.

isNaN is an ES1 spec botch. Behold:

js> isNaN("arfl")
true
js> isNaN({})
true
js> isNaN(null)
false
js> isNaN(undefined)
true
js> isNaN(false)
false
js> isNaN(true)
false

/be
This is because isNaN converts its argument to number, of course:

js> +false
0
js> +true
1
js> +null
0
js> +undefined
NaN
js> +{}
NaN
js> +"arfl"
NaN

/be
Attachment #425105 - Flags: review?(jorendorff) → review+
http://hg.mozilla.org/mozilla-central/rev/b4268cb7bf48
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Why is this marked fixed when it was backed out?

Quote from mg: 
Robert O'Callahan <robert@ocallahan.org> - Thu, 04 Mar 2010 16:36:08 +1300 - rev 38916
Backed out changeset b4268cb7bf48 

I also checked using mg,and mxr and the changes are not in mozilla central.
Yes, I forgot to reopen, sorry.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This will be fixed by the patch in bug 550351.
Depends on: 550351
Whiteboard: fixed-in-tracemonkey
Blocks: 551326
Or rather, bug 557728, which landed on trunk.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.