The default bug view has changed. See this FAQ.

Remove JS_HAS_GENERATORS #define

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Paul Biggar, Assigned: wingo)

Tracking

Trunk
mozilla25
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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.

Comment 2

6 years ago
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

6 years ago
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

6 years ago
Created attachment 525295 [details] [diff] [review]
Updated patch with better jsversion.h comments

Updated patch with better jsversion.h comments
Attachment #525258 - Attachment is obsolete: true

Comment 5

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

Updated

6 years ago
Attachment #525295 - Flags: review+
(Reporter)

Updated

6 years ago
Whiteboard: [good first bug]

Comment 6

6 years ago
My account is still in the process of being created, can someone push a try build for me for this?
(Reporter)

Comment 7

6 years ago
I'll push this for you. Am just updating the patch a bit first.
(Reporter)

Comment 8

6 years ago
http://tbpl.mozilla.org/?tree=Try&rev=deee86a01ce3
(Reporter)

Comment 9

6 years ago
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

6 years ago
Got my account now.  Fixed up the patch and did a try build:
http://tbpl.mozilla.org/?tree=Try&rev=37d95a171a71

Comment 11

6 years ago
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?
Attachment #525295 - Attachment is obsolete: true
(Reporter)

Comment 12

6 years ago
(In reply to comment #11)
> Perhaps I should be doing this against the trace monkey repo instead of m-c?

Yes.
(Reporter)

Updated

6 years ago
Blocks: 666042
JP, are you still working on this?
Assignee: general → wingo
(Assignee)

Comment 14

4 years ago
Created attachment 764708 [details] [diff] [review]
Remove uses of HAS_JS_GENERATORS
(Assignee)

Comment 15

4 years ago
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
(Assignee)

Updated

4 years ago
Attachment #764708 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

4 years ago
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.  :-)
(Assignee)

Comment 18

4 years ago
Created attachment 766573 [details] [diff] [review]
Remove HAS_JS_GENERATORS #define
(Assignee)

Updated

4 years ago
Attachment #764708 - Attachment is obsolete: true
(Assignee)

Comment 19

4 years ago
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?
(Assignee)

Comment 21

4 years ago
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+
(Assignee)

Comment 23

4 years ago
Created attachment 767686 [details] [diff] [review]
Remove HAS_JS_GENERATORS #define
(Assignee)

Updated

4 years ago
Attachment #766573 - Attachment is obsolete: true
(Assignee)

Comment 24

4 years ago
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/87c8917dda60
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/87c8917dda60
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.