Closed Bug 771743 Opened 9 years ago Closed 9 years ago

Get rid of the generator_op mega-method

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: Waldo, Assigned: Waldo)

Details

(Whiteboard: [js:t])

Attachments

(1 file)

Attached patch PatchSplinter Review
The algorithms for the next/send/close/throw methods are much clearer when all spelled out separately, rather than in one intertwined unit.  Plus this will make future work on non-generic method guarding easier to do.
Attachment #639908 - Flags: review?(luke)
Whiteboard: [js:t]
Comment on attachment 639908 [details] [diff] [review]
Patch

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

Ah, I can read it now.

::: js/src/jsiter.cpp
@@ +1626,4 @@
>      }
>  
> +    if (!SendToGenerator(cx, JSGENOP_SEND, thisObj, gen,
> +                         args.length() > 0 ? args[0] : UndefinedValue()))

Could we add a args.argOrUndefined(i) for this common pattern?  It would let this be a 1 liner and much less gross-looking.

@@ +1673,5 @@
> +
> +    JSGenerator *gen = (JSGenerator *) thisObj->getPrivate();
> +    if (!gen || gen->state == JSGEN_CLOSED) {
> +        /* This happens when obj is the generator prototype. See bug 352885. */
> +        cx->setPendingException(args.length() >= 1 ? args[0] : UndefinedValue());

And you could use it here.
Attachment #639908 - Flags: review?(luke) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfab02971a08

Given we want to HANDLE ALL THE THINGS eventually, and I don't think such a method is Handle-ready(tm), I'm not sure if adding it would be forwardly-compatible.  (Not that this is any more compatible, but...)  Dunno what exactly to do about this, landing as-is and we can figure out if we want to do something different at our leisure.
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/dfab02971a08
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.