Refactor Iterator/Generator/StopIteration initialization

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Created attachment 551149 [details] [diff] [review]
Initial rearrangement of js_InitIterator, clarification of Iterator startup
Attachment #551149 - Flags: review?(luke)
Created attachment 551150 [details] [diff] [review]
Simplification of Generator startup
Attachment #551150 - Flags: review?(luke)
Created attachment 551153 [details] [diff] [review]
Simplification of StopIteration startup
Attachment #551153 - Flags: review?(luke)
Blocks: 675520

Comment 3

6 years ago
Comment on attachment 551149 [details] [diff] [review]
Initial rearrangement of js_InitIterator, clarification of Iterator startup

Review of attachment 551149 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsiter.cpp
@@ +1453,5 @@
> +    GlobalObject *global = obj->asGlobal();
> +
> +    /*
> +     * Bail if Iterator has already been initialized.  We test for Iterator
> +     * rather than for StopIteration because if js_InitIteratorClasses recurs,

s/js_//
Attachment #551149 - Flags: review?(luke) → review+

Updated

6 years ago
Attachment #551150 - Flags: review?(luke) → review+

Comment 4

6 years ago
Comment on attachment 551153 [details] [diff] [review]
Simplification of StopIteration startup

Review of attachment 551153 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsiter.cpp
@@ +1445,4 @@
>  static JSObject *
>  InitStopIterationClass(JSContext *cx, GlobalObject *global)
>  {
> +    JSObject *proto = global->createBlankPrototype(cx, &js_StopIterationClass);

Here, I have the opposite comment as below: createBlankPrototype seems like it would be a free function, akin to NewNonFunction.

@@ +1452,5 @@
> +    /* This should use a non-JSProtoKey'd slot, but this is easier for now. */
> +    if (!DefineConstructorAndPrototype(cx, global, JSProto_StopIteration, proto, proto))
> +        return NULL;
> +
> +    MarkStandardClassInitializedNoProto(global, &js_StopIterationClass);

From IRL: some time in the future it would be nice if DefineConstructAndPrototype and MarkStandardClassInitializedNoProto were members of the GlobalObject class.
Attachment #551153 - Flags: review?(luke) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/a399a694bfad
http://hg.mozilla.org/integration/mozilla-inbound/rev/aade388e6c62
http://hg.mozilla.org/integration/mozilla-inbound/rev/f25d125362a8

Re s/js_//: note that that function name remains as it was, so I think that's correct for now.

Re createBlankPrototype: we'll have to ask people.  No one else who's reviewed patches such as these has said anything, and it's not clear to me why homing the method in GlobalObject is not right.

Re IRL: yeah.  There is more work to do even after the patches I have in my tree have landed, and that can be part of it.
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
http://hg.mozilla.org/mozilla-central/rev/a399a694bfad
http://hg.mozilla.org/mozilla-central/rev/aade388e6c62
http://hg.mozilla.org/mozilla-central/rev/f25d125362a8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.