Last Comment Bug 771743 - Get rid of the generator_op mega-method
: Get rid of the generator_op mega-method
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla16
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2012-07-06 19:48 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-07-11 09:32 PDT (History)
2 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (5.92 KB, patch)
2012-07-06 19:48 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review

Description User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-06 19:48:29 PDT
Created attachment 639908 [details] [diff] [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.
Comment 1 User image Luke Wagner [:luke] 2012-07-09 17:49:39 PDT
Comment on attachment 639908 [details] [diff] [review]

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.
Comment 2 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-10 17:53:26 PDT

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.
Comment 3 User image Ed Morley [:emorley] 2012-07-11 09:32:42 PDT

Note You need to log in before you can comment on or make changes to this bug.