Closed Bug 558715 Opened 14 years ago Closed 9 years ago

encapsulate JSSLOT_ITER_* within JSObject

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: n.nethercote, Assigned: gal)

Details

Attachments

(1 file)

new things to come (totally incomplete, just fyi)
21.33 KB, patch
Details | Diff | Splinter Review 
This one's tricky.  In jsiter.h we have:

  const uint32 JSSLOT_ITER_STATE  = JSSLOT_PRIVATE;
  const uint32 JSSLOT_ITER_FLAGS  = JSSLOT_PRIVATE + 1;

which is typical.  But jsapi.cpp also has this:

  const uint32 JSSLOT_ITER_INDEX = JSSLOT_PRIVATE + 1;

which is highly atypical and confuses me greatly.

Is fslots[JSSLOT_PRIVATE+1] "flags" or "index" or both?
ITER_INDEX is bs. I am dealing with this in my patch. Those can indeed move into jsiter.cpp.
Read the code, please -- "foo is bs" is technically content free (and incorrect). JSSLOT_ITER_INDEX is for prop_iter_class, not js_IteratorClass.

The name starting JSSLOT_ITER_* is unfortunate. Usually the class name would be abbreviated in the JSSLOT_FOO_ prefix.

mrbkap can talk about the JS_NewPropertyIterator API. It may not have reached stable orbit, in which case we should remove it.

/be
ITER_INDEX is, if memory serves, going away in my getOwnPropertyNames work too -- funtimes with merge conflicts.  :-\
(In reply to comment #3)
> ITER_INDEX is, if memory serves, going away in my getOwnPropertyNames work too
> -- funtimes with merge conflicts.  :-\

That is part of the territory, especially if you take long enough (I am on the other side of this due to my own slowness, so don't get me wrong -- I sympathize).

But first things first: what about JS_NewPropertyIterator? Can we lose it and simplify merges to just "remove the whole conflicting area"?

/be
(In reply to comment #2)
> Read the code, please -- "foo is bs" is technically content free (and
> incorrect). JSSLOT_ITER_INDEX is for prop_iter_class, not js_IteratorClass.
> 
> The name starting JSSLOT_ITER_* is unfortunate. Usually the class name would be
> abbreviated in the JSSLOT_FOO_ prefix.
> 
> mrbkap can talk about the JS_NewPropertyIterator API. It may not have reached
> stable orbit, in which case we should remove it.

I'll encapsulate ITER_STATE and ITER_FLAGS now, and worry about ITER_INDEX later, which will hopefully give someone else enough time to remove it.  Thanks for the info.
Please don't touch STATE and FLAGS. All that code is becoming obsolete.
Assignee: nnethercote → gal
(I am rewriting the iterator code)
Gal, if you're doing that can you encapsulate the new slots up front (eg. make JSSLOT_ITER_* private in JSObject and add getIterBegin(), setIterBegin(), etc)?  That'll save me doing it afterwards.

I see you're using {set,get}Slot() but that involves an fslots test, so per-slot getters/setters would be better.
fslots test?
its a constant slot, so any conditional expression on slots becomes constant as long we inline this
(In reply to comment #11)
> its a constant slot, so any conditional expression on slots becomes constant as
> long we inline this

Hmm, true.  Can you do it for consistency then? :)
Also, it lets you assert that the object is really an Iterator.
Not sure what you want me to do? We have dozens and dozens of internal slots. I don't want all of those to pollute JSObject. Do you want setDateSlot and setIterSlot and setCallSlot and setWithSlot and setFileSlot and setXPGarbageSlot as well?
(In reply to comment #14)
> Not sure what you want me to do? We have dozens and dozens of internal slots. I
> don't want all of those to pollute JSObject. Do you want setDateSlot and
> setIterSlot and setCallSlot and setWithSlot and setFileSlot and
> setXPGarbageSlot as well?

Yes.  We're already halfway there, with getters/setters for Arrays and Arguments, and I filed this bug for Iterators, and I'm planning to do it for the remaining cases (of which there are about one dozen, not dozens and dozens).  They're encapsulation, not pollution.
Well you only see part of the equation. XPConnect and other embeddings use the same interface. I don't think the generic JSObject should have date-object specific methods. I am ok with parent and proto because that all objects share. Lets add a couple peers and see what people think, I might be wrong.
We had a bit o' discussion around the pit and (although I didn't originally think so) IMHO

  current < encapsulated/polluted JSObject < subclassing

where subclassing means something like:

class JSObject {
  ...
  inline bool isIterObject() const;
  JSIterObject *asIterObject() const { return (JSIterObject *)this; }
};

class JSIterObject : JSObject {
  /* no data members */
  static const unsigned JSSLOT_ITER_BEGIN = ...;
 public:
  getIterBegin();
  setIterBegin();
  ...
};

Even the isIterObject/asIterObject is optional, but I think it would both save keystrokes and assert correct use of asX().  With such a design, subclasses could run wild and the JSObject would stay relatively short.  That's a bigger change, though.  What Nick is doing/proposing seems like a positive step, regardless.
(In reply to comment #17) something like:
> 
> class JSObject {
>   ...
>   inline bool isIterObject() const;
>   JSIterObject *asIterObject() const { return (JSIterObject *)this; }
> };
> 
> class JSIterObject : JSObject {
>   /* no data members */
>   static const unsigned JSSLOT_ITER_BEGIN = ...;
>  public:
>   getIterBegin();
>   setIterBegin();
>   ...
> };

What does an actual use look like?

  jsval x = obj->asIterObject()->getIterBegin()

Clearly some shortening would be in order:

  jsval x = obj->asIter()->getBegin()

vs. the current:

  jsval x = obj->getIterBegin()

I can see it's a little more elegant than my approach, but I'm not convinced it's worth the extra typing, especially with those extra parentheses and arrows.

There are about 10 builtin classes:  array, arguments, primitive (encompassing boolean/number/string), regexp, iterator, date, some xml ones, NoSuchMethod, that's most of them.

I suspect Andreas will argue that both these approaches is too much, that there shouldn't be class-specific methods in JSObject, that we should just use getSlot(JSSLOT_XYZ_FIELD).  That works fine for the fixed reserved slots, but it's no good for the builtin classes that use dslots, eg. Array and Arguments use it for elements.  So you need something like getArrayElem(i) if you want to encapsulate the fslots/dslots split, which was the whole motivation for this stuff, and something I will fight tooth and nail in favour of.
A convincing argument against subclassing would be if all or most places in the code had to write:

  jsval x = obj->asIter()->getBegin()

and code rarely was able to write:

  JSIterObject *iter = obj->asIter();

followed by multiple uses of 'iter'.  If its mostly the former, then subclassing is just more typing and only an warm fuzzy ontological feeling as consolation.
You don't always downcast. You sometimes create and use as the narrower type. Also you get the methods for that type only on that type, so if you can pass the narrower type to subroutines/sub-methods with the narrower signature, you win.

We talked more on IRC. I want njn's encap to move forward, even if it means Date methods in XPConnect's poor face. We can subtype after, with benefit of the encap work and experience using it.

/be
w.r.t. the sub-classing:  polluting JSObject with asFoo() methods isn't ideal.  One way around it would be to make asFoo() a static method of FooObject.

  jsval x = FooObject::as(obj)->getBar()

But maybe that's a net uglification?  Just a thought.
FooObject::of(obj) reads better to me, but I sort of want it to be a cast at that point?
(In reply to comment #21)
> w.r.t. the sub-classing:  polluting JSObject with asFoo() methods isn't ideal. 

Maybe I'm missing something, but isn't the current alternative of polluting JSObject with every class-slot-specific-method in existence far worse than this non-ideal?

(But don't get me wrong: we can finish this up now and still be better off than the status quo without addressing this concern first.)




(In reply to comment #19)
> If its mostly the former, then subclassing is just more typing and only an
> warm fuzzy ontological feeling as consolation.

This isn't just a feeling.  Consider that, instead of having to find one method among dozens, you instead find one as* method among a handful -- once for that object in the scope (maybe twice or worst-case thrice if the object is an array), in a very predictable manner -- and one method among another small handful all directly relevant to the particular class of object you have.  I think it's clear which one works better for, say, the purpose of autocomplete, or of surveying class declarations.
(In reply to comment #23)
> 
> (But don't get me wrong: we can finish this up now and still be better off than
> the status quo without addressing this concern first.)

Exactly.
I don't consider bar->asFoo() bad. I've written code that way in Elsa and it didn't feel too painful. It also seems to be better than any of the alternatives I've seen.
This is totally out-of-date now. The sub-classing that Luke mentioned in comment 17 occurred a long time ago.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: