551007 - Make JSScopeProperty::attrs private, hide information behind accessors

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

jorendorff points out that our JSPROP_READONLY handling interferes with our handling of setting properties with setter functions.  In ES5 accessor descriptors don't have a [[Writable]] field, and only data descriptors do.  Therefore, if we'd like JSScopeProperty::writable() to (eventually) assert that it's only called on data descriptors (we would, it'd be a good check of type-safety), we need to only access this information via accessor functions, and the easiest way to ensure that is to make the field private.

I also took the opportunity to kill the STUB_{G,S}ETTER macros.  Comments and debug output may still refer to stub-ness, but I'm not too worried, we'll get it all eventually.

Anyway, it's definitely best to get this cleanup done in a separate patch from the one which will fix the bug just noted.

Comment 1

[Jason Orendorff [:jorendorff]]

In DropWatchPointAndUnlock:
>-        if (wprop &&
>-            ((wprop->attrs ^ sprop->attrs) & JSPROP_SETTER) == 0 &&
>+        if (wprop && wprop->hasSetter() == sprop->hasSetter() &&
>             IsWatchedProperty(cx, wprop)) {

When the condition doesn't fit on a single line, the prevailing style is to put
each operand of && on a separate line, like this:

          if (wprop &&
              wprop->hasSetter() == sprop->hasSetter() &&
              IsWatchedProperty(cx, wprop)) {

Same comment in a few other places: js_FillPropertyCache twice,
jsops.cpp JSOP_SETMETHOD once.

In js_HasOwnProperty:
>-            if (!SPROP_IS_SHARED_PERMANENT((JSScopeProperty *) *propp)) {
>+            JSScopeProperty *sprop = reinterpret_cast<JSScopeProperty *>(*propp);
>+            if (!sprop->slotlessAndNotConfigurable()) {

Let's not make this change. SHARED_PERMANENT is an allcaps reminder that what
we're testing for here is the shared-permanent hack (for those that know about
it). The new method name, slotlessAndNotConfigurable, is by comparison just
kind of baffling.  Let's keep the macro for now, redefining it in terms of
slotless() and configurable(); and get rid of it when we get rid of the hack.

In js_SetPropertyHelper:
>             /* Don't clone a shared prototype property. */
>-            if (attrs & JSPROP_SHARED) {
>+            if (sprop->slotless()) {

Update the comment too?

In struct JSScopeProperty:
>+    bool hasGetter() const { return attrs & JSPROP_GETTER; }
>+    bool hasSetter() const { return attrs & JSPROP_SETTER; }

These names are misleading. For an sprop with a native getter, sprop->hasGetter() is false. And it's weird how hasGetter() implies !hasDefaultGetter().

Here are some counterproposals. Take your pick:
   hasGetterObject
   hasObjectGetter  <--- probably my favorite
   hasGetterValue        (goes nicely with getterValue method)
   hasES5Getter
   hasECMAGetter

slotless() is a problem too, because most of the places checking it are not
really asking if the property has a slot. They're asking what the behavior
should be when the property is inherited and assigned. If it's SHARED, we call
the inherited setter. Otherwise we ignore the inherited property and make a new
own data property.

A lesser problem with slotless() is the double negative in !sprop->slotless().
The easy fix for that is to flip the meaning and call it hasSlot().

I don't know what to do about this stuff. Attrs are a zoo. :-P  Your call.

I suggest also changing stuff like this, even though it's not necessary to get
the code to compile:

    JSObject *getterObject() const {
        JS_ASSERT(attrs & JSPROP_GETTER);   <--- use the method instead here
        return js_CastAsObject(rawGetter);
    }

Also your call.

r=me with the other changes.

Comment 2

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/48bb07b49e1f

with requested changes (including suggestions), and has{G,S}etterValue.

Comment 3

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/48bb07b49e1f