Closed Bug 771026 Opened 8 years ago Closed 8 years ago

Kill JSProperty and replace with rooted shapes

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: billm, Assigned: billm)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
As far as I can tell, JSProperty always holds a Shape or the values 0 or 1. When it holds 1, we're always dealing with non-native objects where we never look at the shape anyway. So we could replace 1 with anything as long as it's not 0, which denotes "not found."

This patch converts every instance of JSProperty to Shape. And it roots the Shape and passes it through handles. In the places where we used to use (JSProperty*)1, I instead just stuck an arbitrary Shape. I think this is okay.
Attachment #639201 - Flags: review?(bhackett1024)
Comment on attachment 639201 [details] [diff] [review]
patch

Review of attachment 639201 [details] [diff] [review]:
-----------------------------------------------------------------

Double yay!  We should have holiday traditions of shoveling obsolete code out of the engine.

::: js/src/jsarray.cpp
@@ +728,5 @@
>      if (!obj->isDenseArray())
>          return baseops::LookupElement(cx, obj, index, objp, propp);
>  
>      if (IsDenseArrayIndex(obj, index)) {
> +        propp.set(obj->lastProperty()); /* just need to set to something non-null */

This and other non-native lookups should use some unwieldy common function like MarkNonNativeLookupSuccess, with a comment explaining what's going on.
Attachment #639201 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/fe305819d2f2
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Shouldn't this be reopened?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla16 → ---
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/f88cf68c7fcd
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.