Closed Bug 648949 Opened 12 years ago Closed 10 years ago

Remove JS_HAS_GENERATORS #define


(Core :: JavaScript Engine, defect)

Not set





(Reporter: paul.biggar, Assigned: wingo)




(1 file, 5 obsolete files)

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.
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.
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.
Built with JS_VERSION=170 as well as without the env var set and both compile and link fine for me on linux.
Updated patch with better jsversion.h comments
Attachment #525258 - Attachment is obsolete: true
The local style says } else { not
else {
(you removed an #ifdef in the middle of it)
Attachment #525295 - Flags: review+
Whiteboard: [good first bug]
My account is still in the process of being created, can someone push a try build for me for this?
I'll push this for you. Am just updating the patch a bit first.
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.
Got my account now.  Fixed up the patch and did a try build:
Perhaps I should be doing this against the trace monkey repo instead of m-c?
Attachment #525295 - Attachment is obsolete: true
(In reply to comment #11)
> Perhaps I should be doing this against the trace monkey repo instead of m-c?

Blocks: 666042
JP, are you still working on this?
Assignee: general → wingo
Attached patch Remove uses of HAS_JS_GENERATORS (obsolete) — Splinter Review
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).
Attachment #529475 - Attachment is obsolete: true
Attachment #764708 - Flags: review?(jwalden+bmo)
Blocks: 884794
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) \
Attachment #764708 - Flags: review?(jwalden+bmo) → review+
Oh, you have more jorendorff coordination re bug 883333 to do here.  :-)
Attached patch Remove HAS_JS_GENERATORS #define (obsolete) — Splinter Review
Attachment #764708 - Attachment is obsolete: true
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.
Attachment #766573 - Flags: review?(jwalden+bmo)
Attachment #766573 - Flags: review?(jorendorff)
Comment on attachment 766573 [details] [diff] [review]
Remove HAS_JS_GENERATORS #define

Why you reversed the jsversion.h change?
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.
Attachment #766573 - Flags: review?(jwalden+bmo) → review+
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 (...) {
Attachment #766573 - Flags: review?(jorendorff) → review+
Attachment #766573 - Attachment is obsolete: true
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!
Keywords: checkin-needed
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.