Last Comment Bug 648949 - Remove JS_HAS_GENERATORS #define
: Remove JS_HAS_GENERATORS #define
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla25
Assigned To: Andy Wingo [:wingo]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 666042 884794
  Show dependency treegraph
 
Reported: 2011-04-10 22:08 PDT by Paul Biggar
Modified: 2013-06-26 13:41 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First pass at removing dead defines and related code. (105.08 KB, patch)
2011-04-11 18:26 PDT, JP Rosevear [:jpr]
no flags Details | Diff | Splinter Review
Updated patch with better jsversion.h comments (105.89 KB, patch)
2011-04-11 21:24 PDT, JP Rosevear [:jpr]
gal: review+
Details | Diff | Splinter Review
Updated patch to handle new #define additions. (106.85 KB, patch)
2011-05-02 07:48 PDT, JP Rosevear [:jpr]
no flags Details | Diff | Splinter Review
Remove uses of HAS_JS_GENERATORS (25.25 KB, patch)
2013-06-19 05:47 PDT, Andy Wingo [:wingo]
jwalden+bmo: review+
Details | Diff | Splinter Review
Remove HAS_JS_GENERATORS #define (27.34 KB, patch)
2013-06-24 01:19 PDT, Andy Wingo [:wingo]
jwalden+bmo: review+
jorendorff: review+
Details | Diff | Splinter Review
Remove HAS_JS_GENERATORS #define (28.60 KB, patch)
2013-06-26 03:37 PDT, Andy Wingo [:wingo]
no flags Details | Diff | Splinter Review

Description Paul Biggar 2011-04-10 22:08:28 PDT
JS_HAS_GENERATORS should be permanently on. There is no easy way to turn it off, and I expect it wouldn't work if we could.
Comment 1 Phil Ringnalda (:philor) 2011-04-10 22:55:55 PDT
The easy way to turn it off is "export JS_VERSION=160 && configure && make" (which does indeed fail to build, in at least a couple of places) so along with the easy part of this bug, just removing the #ifs, you'll need to make some alterations in js/src/jsversion.h to reflect that everything below 170 is no longer supported, which probably means JS_HAS_BLOCK_SCOPE and JS_HAS_DESTRUCTURING are also due for the chopping block, since they look like they're in the same state of only getting set if you try to build 160.
Comment 2 JP Rosevear [:jpr] 2011-04-11 18:26:30 PDT
Created attachment 525258 [details] [diff] [review]
First pass at removing dead defines and related code.

First pass at removing dead defines and related code, including JS_HAS_SPARSE_ARRAYS which was 0 everywhere.

In jsversion.h support for < 1.7 has been removed.
Comment 3 JP Rosevear [:jpr] 2011-04-11 21:23:18 PDT
Built with JS_VERSION=170 as well as without the env var set and both compile and link fine for me on linux.
Comment 4 JP Rosevear [:jpr] 2011-04-11 21:24:11 PDT
Created attachment 525295 [details] [diff] [review]
Updated patch with better jsversion.h comments

Updated patch with better jsversion.h comments
Comment 5 Andreas Gal :gal 2011-04-11 21:47:46 PDT
The local style says } else { not
}
else {
(you removed an #ifdef in the middle of it)
Comment 6 JP Rosevear [:jpr] 2011-04-26 12:05:22 PDT
My account is still in the process of being created, can someone push a try build for me for this?
Comment 7 Paul Biggar 2011-04-26 12:29:38 PDT
I'll push this for you. Am just updating the patch a bit first.
Comment 9 Paul Biggar 2011-04-26 16:11:48 PDT
This try is pretty red and orange, I likely made a mistake fixing the patch. Can you update the patch and I can try again.
Comment 10 JP Rosevear [:jpr] 2011-05-02 07:46:41 PDT
Got my account now.  Fixed up the patch and did a try build:
http://tbpl.mozilla.org/?tree=Try&rev=37d95a171a71
Comment 11 JP Rosevear [:jpr] 2011-05-02 07:48:22 PDT
Created attachment 529475 [details] [diff] [review]
Updated patch to handle new #define additions.

Perhaps I should be doing this against the trace monkey repo instead of m-c?
Comment 12 Paul Biggar 2011-05-02 16:46:19 PDT
(In reply to comment #11)
> Perhaps I should be doing this against the trace monkey repo instead of m-c?

Yes.
Comment 13 Ed Morley [:emorley] 2011-08-27 10:29:23 PDT
JP, are you still working on this?
Comment 14 Andy Wingo [:wingo] 2013-06-19 05:47:33 PDT
Created attachment 764708 [details] [diff] [review]
Remove uses of HAS_JS_GENERATORS
Comment 15 Andy Wingo [:wingo] 2013-06-19 05:51:45 PDT
Comment on attachment 529475 [details] [diff] [review]
Updated patch to handle new #define additions.

Marking previous patch as obsolete; happily jsversion.h is much less horrible now.

My patch removes uses of JS_HAS_GENERATORS but not the JS_HAS_GENERATORS definition; I can remove that too if that's the thing.  With ES6, generators are always on (though in a slightly different form; see bug 666399).
Comment 16 Jeff Walden [:Waldo] (remove +bmo to email) 2013-06-21 09:56:36 PDT
Comment on attachment 764708 [details] [diff] [review]
Remove uses of HAS_JS_GENERATORS

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

::: js/src/vm/Keywords.h
@@ +26,5 @@
>        macro(let, let, TOK_STRICT_RESERVED, JSOP_NOP, JSVERSION_1_7)
>  #endif
> +
> +#define FOR_YIELD_KEYWORD(macro) \
> +    macro(yield, yield, TOK_YIELD, JSOP_NOP, JSVERSION_1_7)

As this is no longer conditional, please move it into a section in FOR_EACH_JAVASCRIPT_KEYWORD(macro):

    /* ES5 future reserved keyword in strict mode, keyword in JS1.7 even when not strict. */ \
    macro(yield, yield, TOK_YIELD, JSOP_NOP, JSVERSION_1_7) \
Comment 17 Jeff Walden [:Waldo] (remove +bmo to email) 2013-06-21 10:26:02 PDT
Oh, you have more jorendorff coordination re bug 883333 to do here.  :-)
Comment 18 Andy Wingo [:wingo] 2013-06-24 01:19:26 PDT
Created attachment 766573 [details] [diff] [review]
Remove HAS_JS_GENERATORS #define
Comment 19 Andy Wingo [:wingo] 2013-06-24 01:23:27 PDT
Comment on attachment 766573 [details] [diff] [review]
Remove HAS_JS_GENERATORS #define

Rebased patch, and inlined the yield case.  Happily there was no fallout from bug 883333.
Comment 20 Masatoshi Kimura [:emk] 2013-06-24 19:37:37 PDT
Comment on attachment 766573 [details] [diff] [review]
Remove HAS_JS_GENERATORS #define

Why you reversed the jsversion.h change?
Comment 21 Andy Wingo [:wingo] 2013-06-25 02:47:46 PDT
Not sure I understand you emk.  jsversion.h changed a lot between 2011 and now; I didn't start from jpr's patches, I merely found a bug that was open.  jsversion.h is fine as it is, AFAICT.
Comment 22 Jason Orendorff [:jorendorff] 2013-06-25 11:58:07 PDT
Comment on attachment 766573 [details] [diff] [review]
Remove HAS_JS_GENERATORS #define

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

::: js/src/jsatom.cpp
@@ -105,3 @@
>  const char js_close_str[]           = "close";
>  const char js_send_str[]            = "send";
> -#endif

Please re-alphabetize the list.

::: js/src/jsatom.h
@@ +174,5 @@
>  extern const char js_while_str[];
>  extern const char js_with_str[];
>  extern const char js_yield_str[];
> +extern const char js_close_str[];
> +extern const char js_send_str[];

Same here.

::: js/src/jsiter.cpp
@@ +1020,1 @@
>      else if (obj->isGenerator()) {

join this with the previous line, per the prevailing style:

   } else if (...) {
Comment 23 Andy Wingo [:wingo] 2013-06-26 03:37:48 PDT
Created attachment 767686 [details] [diff] [review]
Remove HAS_JS_GENERATORS #define
Comment 24 Andy Wingo [:wingo] 2013-06-26 03:43:29 PDT
Updated patch is now properly rebased (the pull -u after qpop -a had failed before for strange reasons) and addresses comment 22.  Setting checkin-needed; thanks for the review!
Comment 25 Ryan VanderMeulen [:RyanVM] 2013-06-26 06:48:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/87c8917dda60
Comment 26 Ryan VanderMeulen [:RyanVM] 2013-06-26 13:41:35 PDT
https://hg.mozilla.org/mozilla-central/rev/87c8917dda60

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