Skipping holes and undefs while sorting

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
enhancement
VERIFIED FIXED
13 years ago
12 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

Trunk
x86
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

13 years ago
The current implementation of array_sort in jsarray.c includes in the temporally
sorting array unexisting properties (holes) and undefined values and later put
some efforts in sort_compare to ensure hole > undef > any other value.

This can be simplified if holes and undefs would not be included in the working
array but rather just counted. Then after the sorting is done undefs and holes
can be properly inserted at the array end.
(Assignee)

Comment 1

13 years ago
Created attachment 198802 [details] [diff] [review]
Implementation

Comment 2

13 years ago
not sure if we have a test for sorting holes/undefined, so just adding this for completeness.

Checking in regress-311515.js;
/cvsroot/mozilla/js/tests/js1_5/Array/regress-311515.js,v  <--  regress-311515.js
initial revision: 1.1
done
Flags: testcase+
(Assignee)

Comment 3

13 years ago
Comment on attachment 198802 [details] [diff] [review]
Implementation

This is a wrong patch
Attachment #198802 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Assignee: general → igor.bukanov
(Assignee)

Comment 4

13 years ago
Created attachment 203678 [details] [diff] [review]
Implementation
Attachment #203678 - Flags: review?(brendan)
Comment on attachment 203678 [details] [diff] [review]
Implementation

Deflecting to mrbkap, who has some jsarray.c major patching under way.

/be
Attachment #203678 - Flags: review?(brendan) → review?(mrbkap)
(Assignee)

Comment 6

12 years ago
Created attachment 215139 [details] [diff] [review]
Implementation (sync with CVS 2006-03-15)
Attachment #203678 - Attachment is obsolete: true
Attachment #215139 - Flags: review?(mrbkap)
Attachment #203678 - Flags: review?(mrbkap)
Comment on attachment 215139 [details] [diff] [review]
Implementation (sync with CVS 2006-03-15)

>Index: jsarray.c
>+     * By ECMA 262, 15.4.4.11, undefined property (which we call a "hole")
>+     * always bigger an existing property with value undefined and that

You're missing a couple of words here :-)
By ECMA 262, 15.4.4.11, an undefined property (which we call a "hole") is always bigger than an existing property ...

Looks good otherwise, r=mrbkap.
Attachment #215139 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 8

12 years ago
Created attachment 215165 [details] [diff] [review]
Patch to commit with comments fixes.
Attachment #215139 - Attachment is obsolete: true
(Assignee)

Comment 9

12 years ago
I committed the last patch:

Checking in jsarray.c;
/cvsroot/mozilla/js/src/jsarray.c,v  <--  jsarray.c
new revision: 3.76; previous revision: 3.75
done
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Comment on attachment 215165 [details] [diff] [review]
Patch to commit with comments fixes.

>+     * By ECMA 262, 15.4.4.11, an undefined property (which we call a "hole")
>+     * always bigger than an existing property with value undefined and that

Missing "is" before "always bigger".

Also "greater" is better than "bigger", more Mathematical.

>+     * is always bigger then any other property. Thus to sort holes and

s/then/than/

r=me in advance for followup fix if you are game.

/be
(Assignee)

Comment 11

12 years ago
I committed the spelling fixes:

Checking in jsarray.c;
/cvsroot/mozilla/js/src/jsarray.c,v  <--  jsarray.c
new revision: 3.77; previous revision: 3.76
done
(Assignee)

Comment 12

12 years ago
Created attachment 215216 [details] [diff] [review]
Patch with spelling fixes

For references here is the patch with all spelling fixes incorporated.
Attachment #215165 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Blocks: 330812

Comment 13

12 years ago
verified fixed 20060609 win/macppc/linux trunk.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.