Closed
Bug 1175111
Opened 9 years ago
Closed 9 years ago
Wrong property order in Object.getOwnPropertyNames(string) and others
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla44
People
(Reporter: 446240525, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
6.92 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
js> Object.getOwnPropertyNames("123") ["length", "0", "1", "2"] // should be [ "0", "1", "2", "length" ], per spec 9.4.3.3
Comment 1•9 years ago
|
||
Rules about own property ordering affect many builtins: - Object.keys - Object.getOwnPropertyNames - Object.assign - JSON.stringify - Reflect.ownKeys - Reflect.enumerate (not yet implemented, bug 1185493) There may also be aspects of behavior exposed by the for-in loop alone that we get wrong.
Summary: The order of properties returned by Object.getOwnPropertyNames(string) is wrong → Wrong property order in Object.getOwnPropertyNames(string) and others
Assignee | ||
Comment 2•9 years ago
|
||
This fixes for-in, Object.getOwnPropertyNames etc to sort integer properties and put them before other properties. The only problem I see is that the standard defines an integer property as any integer >= 0 and < 2**53, whereas our "indexed id" range is much smaller. This seems to be a more general issue though and I think fixing this requires changes to IdIsIndex etc.
Comment 3•9 years ago
|
||
Comment on attachment 8676399 [details] [diff] [review] Patch Review of attachment 8676399 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: js/src/jsiter.cpp @@ +144,5 @@ > +{ > + uint32_t indexA, indexB; > + DebugOnly<bool> isIndexA = IdIsIndex(a, &indexA); > + DebugOnly<bool> isIndexB = IdIsIndex(b, &indexB); > + MOZ_ASSERT(isIndexA && isIndexB); You can use MOZ_ALWAYS_TRUE for these. @@ +165,2 @@ > for (size_t i = 0; i < initlen; ++i, ++vp) { > if (!vp->isMagic(JS_ELEMENTS_HOLE)) { Maybe swap the two arms of this if-else now, and remove the `!`.
Attachment #8676399 -
Flags: review?(jorendorff) → review+
Comment 5•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/45dd451ce4ed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
I'm sure the array part of this is my fault -- sorry!
You need to log in
before you can comment on or make changes to this bug.
Description
•