Last Comment Bug 648355 - Function.prototype.isGenerator
: Function.prototype.isGenerator
Status: RESOLVED FIXED
fixed-in-tracemonkey
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Dave Herman [:dherman]
:
Mentors:
http://wiki.ecmascript.org/doku.php?i...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-07 13:49 PDT by Dave Herman [:dherman]
Modified: 2011-04-26 05:37 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Function.isGenerator (2.41 KB, patch)
2011-04-07 17:55 PDT, Dave Herman [:dherman]
no flags Details | Diff | Splinter Review
moved to Function.prototype, added tests (3.43 KB, patch)
2011-04-08 13:00 PDT, Dave Herman [:dherman]
brendan: review+
Details | Diff | Splinter Review

Description Dave Herman [:dherman] 2011-04-07 13:49:09 PDT
Function.isGenerator(f) -> boolean

Indicates whether a given function is a generator function. As specified on the attached TC39 wiki page.

Should be easy -- I'll try to do today.

Dave
Comment 1 Dave Herman [:dherman] 2011-04-07 17:55:57 PDT
Created attachment 524551 [details] [diff] [review]
Function.isGenerator

This version tests for generator functions by testing whether a function a) is interpreted and b) starts its script with JSOP_GENERATOR. This seems a bit brittle. Is it worth nabbing a JSFUN_XXX bit and setting that on the JSScript flags?

Dave
Comment 2 Dave Herman [:dherman] 2011-04-07 17:59:54 PDT
Brendan: what was the reason we couldn't put isGenerator on Function.prototype as a (non-enumerable) zero-argument self-test? Does it create web incompatibilities?

Dave
Comment 3 Dave Herman [:dherman] 2011-04-08 13:00:23 PDT
Created attachment 524713 [details] [diff] [review]
moved to Function.prototype, added tests

Ready for review.

Dave
Comment 4 Brendan Eich [:brendan] 2011-04-08 13:11:24 PDT
Comment on attachment 524713 [details] [diff] [review]
moved to Function.prototype, added tests

>From: Dave Herman <dherman@mozilla.com>
>Function.isGenerator (bug 648355, r=?)
>
>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp
>--- a/js/src/jsfun.cpp
>+++ b/js/src/jsfun.cpp
>@@ -2331,16 +2331,39 @@ CallOrConstructBoundFunction(JSContext *
>         return false;
> 
>     *vp = args.rval();
>     return true;
> }
> 
> }
> 
>+    bool rval = false;

Nit: rval is a canonical name for a jsval or js::Value local. Suggest result or answer instead.

>+    if (fun->isInterpreted()) {
>+        JSScript *script = fun->u.i.script;
>+        rval = script->length > 0 && script->code[0] == JSOP_GENERATOR;

Nit: prevailing style prefers != 0 for unsigned tests, not > 0, but:

Non-nit: we don't need a length check here, scripts by definition have at least JSOP_STOP as a bytecode. So you could get rid of the script local, or keep it and assert that length != 0.

>+script regress-648355.js

I'm glad to have a regression test but this space in the suite is for actual bugs we don't want to reopen, not features being added and covered. IIRC from discussion with Waldo, something like tests/js1_8_5/extensions/is-generator.js would be better.

Best to update the strawman at the bug's URL to match what this patch implements.

r=me with these addressed; happy to look again if you like.

/be
Comment 5 Dave Herman [:dherman] 2011-04-08 13:32:19 PDT
http://hg.mozilla.org/tracemonkey/rev/90fa43dcb844
Comment 6 Dave Herman [:dherman] 2011-04-08 13:49:48 PDT
Waldo caught a missing #if/#endif. Pushed here:

http://hg.mozilla.org/tracemonkey/rev/dbecfa435101
Comment 8 Eric Shepherd [:sheppy] 2011-04-26 05:37:02 PDT
Now documented:

https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Function/isGenerator

With a link from the Function page here:

https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Function#Methods_2

And mentioned on Firefox 5 for developers.

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