Closed Bug 551007 Opened 13 years ago Closed 13 years ago

Make JSScopeProperty::attrs private, hide information behind accessors


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)


(Whiteboard: fixed-in-tracemonkey)


(1 file)

Attached patch PatchSplinter Review
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.
Attachment #431194 - Flags: review?(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:
   hasObjectGetter  <--- probably my favorite
   hasGetterValue        (goes nicely with getterValue method)

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.
Attachment #431194 - Flags: review?(jorendorff) → review+

with requested changes (including suggestions), and has{G,S}etterValue.
Whiteboard: fixed-in-tracemonkey
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.