1069416 - Strip out JS_HAS_SYMBOLS #ifdefs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Tom S. (please needinfo tschuster)]

Would be nice if somebody picked this up. After that we can increase XDR_BYTECODE_VERSION_SUBTRAHEND by one instead of two again! https://hg.mozilla.org/mozilla-central/file/86f9d0128ccf/js/src/vm/Xdr.h#l32

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Remove JS_HAS_SYMBOLS.

Prepared 8 patches that remove JS_HAS_SYMBOLS and related things.

Part 1 removes JS_HAS_SYMBOLS from .cpp/.h files in js/src.
Part 2-3 remove similar JS_HAS_SYMBOLS and std_iterator from JS tests.
Part 4 removes JS_HAS_SYMBOLS support from make_opcode_doc.py.
Part 5-8 remove similar variable/code from JS code/tests in other directories.
(not sure whether Part 2-3,5-8 are actually needed or not).

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=09692a34dd76

Comment 3

[Tom S. (please needinfo tschuster)]

Comment on attachment 8554319 [details] [diff] [review]
Part 1: Remove JS_HAS_SYMBOLS.

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

Very nice! The other patches look good as well, please ask for review on those, too. Sadly I probably can't review some of the changes to the browser.

::: js/src/jsprototypes.h
@@ +92,5 @@
>      real(WeakMap,               32,     js_InitWeakMapClass,       OCLASP(WeakMap)) \
>      real(Map,                   33,     js_InitMapClass,           OCLASP(Map)) \
>      real(Set,                   34,     js_InitSetClass,           OCLASP(Set)) \
>      real(DataView,              35,     js_InitDataViewClass,      OCLASP(DataView)) \
> +    real(Symbol,                36,     js_InitSymbolClass,        &js::SymbolObject::class_) \

Can you check if OCLASP(Symbol) works?

::: js/src/vm/Xdr.h
@@ +31,2 @@
>   */
>  static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 230;

Let's increment this one last time by two, just to be sure.

@@ +33,2 @@
>  static const uint32_t XDR_BYTECODE_VERSION =
> +    uint32_t(0xb973c0de - (XDR_BYTECODE_VERSION_SUBTRAHEND));

Remove the parentheses.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Remove JS_HAS_SYMBOLS from js tests.

Thank you for reviewing :D

Part 2 removes JS_HAS_SYMBOLS variable from js tests (and jsHasSymbols from xpconnect test).
std_iterator is removed in Part 3.

btw, there are still a lot of `typeof Symbol === "function"`s, is it better to remove them too?

(In reply to Tom Schuster [:evilpie] from comment #3)
> > +    real(Symbol,                36,     js_InitSymbolClass,        &js::SymbolObject::class_) \
> 
> Can you check if OCLASP(Symbol) works?

Yeah, it works :)

> ::: js/src/vm/Xdr.h
> @@ +31,2 @@
> >   */
> >  static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 230;
> 
> Let's increment this one last time by two, just to be sure.
> 
> @@ +33,2 @@
> >  static const uint32_t XDR_BYTECODE_VERSION =
> > +    uint32_t(0xb973c0de - (XDR_BYTECODE_VERSION_SUBTRAHEND));
> 
> Remove the parentheses.

Fixed locally.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Remove std_iterator from js tests.

Part 3 replaces std_iterator by Symbol.iterator, in js shell tests.

std_iterator is also used in Self-hosted JS code. is it better to replace them too?

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Remove JS_HAS_SYMBOLS support from make_opcode_doc.py.

Part 4 removes JS_HAS_SYMBOLS support from make_opcode_doc.py, which generates "Until bug 1066322 is fixed, ..." paragraph in following page:
  https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Internals/Bytecode

Removed one more unnecessary line from the try run:
>-    version_expr = m.group(1)

Comment 7

[Tom S. (please needinfo tschuster)]

Comment on attachment 8554505 [details] [diff] [review]
Part 2: Remove JS_HAS_SYMBOLS from js tests.

A XPConnect peer should review non-trivial changes to test_xrayToJS.xul.

Comment 8

[Tom S. (please needinfo tschuster)]

Comment on attachment 8554510 [details] [diff] [review]
Part 3: Remove std_iterator from js tests.

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

::: js/src/jit-test/tests/for-of/semantics-08.js
@@ +4,5 @@
>  
>  var g = newGlobal();
>  g.eval(`
>      var obj = {};
> +    obj[${uneval(Symbol.iterator)}] = function () { return this; };

I think that can just be obj[Symbol.iterator]

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Remove JS_HAS_SYMBOLS from CustomizableUI.jsm.

Part 5 removes JS_HAS_SYMBOLS and kIteratorSymbol from CustomizableUI.jsm.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Remove JS_HAS_SYMBOLS from devtools.

Part 6 removes JS_HAS_SYMBOLS and ITERATOR_SYMBOL from devtools.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Remove JS_HAS_SYMBOLS from dom.

Part 7 removes JS_HAS_SYMBOLS, std_iterator, and test cases for old '@@iterator' from tests in dom/.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Remove JS_HAS_SYMBOLS from Promise-backend.js.

Part 8 removes JS_HAS_SYMBOLS and ITERATOR_SYMBOL from Promise-backend.js.

Comment 13

[Tooru Fujisawa [:arai]]

Thank you all :D

https://hg.mozilla.org/integration/mozilla-inbound/rev/cad25450eff5
https://hg.mozilla.org/integration/mozilla-inbound/rev/32e5ed282edb
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4b721149af
https://hg.mozilla.org/integration/mozilla-inbound/rev/5af5deffe0cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/670fb41e2cc4
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c34cc2fa5d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f3d5ec3d30b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3256cb6f0bc9

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cad25450eff5
https://hg.mozilla.org/mozilla-central/rev/32e5ed282edb
https://hg.mozilla.org/mozilla-central/rev/ff4b721149af
https://hg.mozilla.org/mozilla-central/rev/5af5deffe0cf
https://hg.mozilla.org/mozilla-central/rev/670fb41e2cc4
https://hg.mozilla.org/mozilla-central/rev/1c34cc2fa5d9
https://hg.mozilla.org/mozilla-central/rev/5f3d5ec3d30b
https://hg.mozilla.org/mozilla-central/rev/3256cb6f0bc9

Comment 15

[Jason Orendorff [:jorendorff]]

Didn't notice this until it landed. :) Thanks!