search

espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.76k stars 741 forks source link

Fixed array sorting with undefined values #2300

Closed glemco closed 1 year ago

glemco commented 1 year ago

Hopefully fixes #2294

gfwilliams commented 1 year ago

Thanks! So this was tested with tests/test_array_sort.js?

Just to confirm though, this is what you mentioned in https://github.com/espruino/Espruino/issues/2294#issuecomment-1327498738 - so it's not actually compliant with the spec, but it does work in the case of a simple comparison?

glemco commented 1 year ago

I added a test for sorting both with and without a function (not both direction but it should be fine). Yes, pretty much, it handles the case in which the compare function returns NaN. If the value is handled internally, users could move the undefined around. It is compliant if the function doesn't handle undefined values itself (which is what I'd say most users would do).

If that's undesired I can modify it to check for undefined values before even running the compare function.

glemco commented 1 year ago

What can possibly go wrong is that if the compare function is dereferencing an object and that's undefined, it can break (note: this is the current behaviour in espruino already). E.g.:

[undefined, {a:1}, {a:0}].sort((a,b)=>a.a - b.a)

In the current implementation (and also after this PR), users may need to check for undefined while comparing anyway. At this stage it's probably better to strictly follow the specs and don't offer the possibility to sort undefined away from the bottom (which looks like a niche feature to me anyway). I'm waiting for your feedback on this

gfwilliams commented 1 year ago

At this stage it's probably better to strictly follow the specs and don't offer the possibility to sort undefined away from the bottom (which looks like a niche feature to me anyway).

Yes, thanks - I think it's the right idea to just go with the spec too - so just checking jsvIsUndefined on each arg before and returning either -1/1/0 to sort them in the right place would be great

glemco commented 1 year ago

At this stage it's probably better to strictly follow the specs and don't offer the possibility to sort undefined away from the bottom (which looks like a niche feature to me anyway).

Yes, thanks - I think it's the right idea to just go with the spec too - so just checking jsvIsUndefined on each arg before and returning either -1/1/0 to sort them in the right place would be great

Now it should be fine, I'm not checking for the 0 case as I don't really think it'd matter which undefined is going before once two are compared (unless there's a strong performance advantage in adding this case too).

gfwilliams commented 1 year ago

Looks great - thanks!