Simplify NativeSet since it is only called for data properties

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

unspecified
mozilla39
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
This should also let us get rid of Shape::set().
(Assignee)

Comment 1

3 years ago
Created attachment 8576963 [details] [diff] [review]
Rename NativeSet -> NativeSetExistingDataProperty and simplify it since it is only called for data properties. Delete Shape::set. Add comments. No change in behavior
Attachment #8576963 - Flags: review?(efaustbmo)
(Assignee)

Updated

3 years ago
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=a66ba09af1da

Comment 3

3 years ago
Comment on attachment 8576963 [details] [diff] [review]
Rename NativeSet -> NativeSetExistingDataProperty and simplify it since it is only called for data properties. Delete Shape::set. Add comments. No change in behavior

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

Seeing shape::set go is nice. That function had understanding of the workings of the engine at the wrong level of abstraction.

::: js/src/vm/NativeObject.cpp
@@ +2125,5 @@
>          }
> +
> +        // Bizarre: shared (slotless) property with no JSSetterOp. JS code
> +        // can't define such a property, but it can be done through the
> +        // JSAPI. Assignment fails.

Who wants this kind of thing? Can we outlaw it at the API level and simplify our lives? If not, can we leave a comment as to the kind of thing that wants it?

In general, can we do that usage annotation for most of these super bizzare special cases?
Attachment #8576963 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 4

3 years ago
(In reply to Eric Faust [:efaust] from comment #3)
> > +        // Bizarre: shared (slotless) property with no JSSetterOp. JS code
> > +        // can't define such a property, but it can be done through the
> > +        // JSAPI. Assignment fails.
> 
> Who wants this kind of thing? Can we outlaw it at the API level and simplify
> our lives? If not, can we leave a comment as to the kind of thing that wants
> it?

Yeah. I'm sort of doing it on a case-by-case basis, because it's hard. I think you'll like what happens in bug 1138499.

The particular thing that's silly here is that the application should've been marked JSPROP_READONLY when it was defined.

> In general, can we do that usage annotation for most of these super bizzare
> special cases?

Usage annotation? I don't understand.

Anyway if someone takes the trouble of tracking down where these weird things are happening, I hope they change the caller to do something more sensible instead of annotating it.
(Assignee)

Comment 5

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce0ee37e3ca9
Backed out that whole push in https://hg.mozilla.org/integration/mozilla-inbound/rev/c3638d994edd for massive widespread bustage, https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7613fc978d36&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception - something must have changed out from under you.
(Assignee)

Comment 7

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=535bfb09e9e6
(Assignee)

Comment 8

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=b276b5f8d718
(Assignee)

Comment 9

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/069211196603
https://hg.mozilla.org/mozilla-central/rev/069211196603
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.