1142775 - Simplify NativeSet since it is only called for data properties

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

This should also let us get rid of Shape::set().

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Rename NativeSet -> NativeSetExistingDataProperty and simplify it since it is only called for data properties. Delete Shape::set. Add comments. No change in behavior

Comment 2

[Jason Orendorff [:jorendorff]]

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

Comment 3

[Eric Faust [:efaust]]

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?

Comment 4

[Jason Orendorff [:jorendorff]]

(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.

Comment 5

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce0ee37e3ca9

Comment 6

[Phil Ringnalda (:philor)]

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.

Comment 7

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=535bfb09e9e6

Comment 8

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=b276b5f8d718

Comment 9

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/069211196603

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/069211196603