Closed Bug 1142775 Opened 5 years ago Closed 5 years ago

Simplify NativeSet since it is only called for data properties

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(1 file)

This should also let us get rid of Shape::set().
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/069211196603
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.