648949 - Remove JS_HAS_GENERATORS #define

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Paul Biggar]

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)]

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]]

Attachment: 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]]

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]]

Attachment: Updated patch with better jsversion.h comments

Updated patch with better jsversion.h comments

Comment 5

[Andreas Gal :gal]

The local style says } else { not
}
else {
(you removed an #ifdef in the middle of it)

Comment 6

[JP Rosevear [:jpr]]

My account is still in the process of being created, can someone push a try build for me for this?

Comment 7

[Paul Biggar]

I'll push this for you. Am just updating the patch a bit first.

Comment 8

[Paul Biggar]

http://tbpl.mozilla.org/?tree=Try&rev=deee86a01ce3

Comment 9

[Paul Biggar]

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]]

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]]

Attachment: 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]

(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]]

JP, are you still working on this?

Comment 14

[Andy Wingo [:wingo]]

Attachment: Remove uses of HAS_JS_GENERATORS

Comment 15

[Andy Wingo [:wingo]]

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]]

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]]

Oh, you have more jorendorff coordination re bug 883333 to do here.  :-)

Comment 18

[Andy Wingo [:wingo]]

Attachment: Remove HAS_JS_GENERATORS #define

Comment 19

[Andy Wingo [:wingo]]

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]]

Comment on attachment 766573 [details] [diff] [review]
Remove HAS_JS_GENERATORS #define

Why you reversed the jsversion.h change?

Comment 21

[Andy Wingo [:wingo]]

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]]

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]]

Attachment: Remove HAS_JS_GENERATORS #define

Comment 24

[Andy Wingo [:wingo]]

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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87c8917dda60

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87c8917dda60