Closed Bug 1069416 Opened 5 years ago Closed 5 years ago

Strip out JS_HAS_SYMBOLS #ifdefs

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: jorendorff, Assigned: arai)

References

Details

Attachments

(8 files)

No description provided.
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
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
Attachment #8554319 - Flags: review?(evilpies)
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.
Attachment #8554319 - Flags: review?(evilpies) → review+
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.
Attachment #8554505 - Flags: review?(evilpies)
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?
Attachment #8554510 - Flags: review?(evilpies)
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)
Attachment #8554514 - Flags: review?(evilpies)
Attachment #8554514 - Flags: review?(evilpies) → review?(jwalden+bmo)
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.
Attachment #8554505 - Flags: review?(mrbkap)
Attachment #8554505 - Flags: review?(evilpies)
Attachment #8554505 - Flags: review+
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]
Attachment #8554510 - Flags: review?(evilpies) → review+
Attachment #8554505 - Flags: review?(mrbkap) → review+
Part 5 removes JS_HAS_SYMBOLS and kIteratorSymbol from CustomizableUI.jsm.
Attachment #8554687 - Flags: review?(bmcbride)
Part 6 removes JS_HAS_SYMBOLS and ITERATOR_SYMBOL from devtools.
Attachment #8554692 - Flags: review?(nfitzgerald)
Part 7 removes JS_HAS_SYMBOLS, std_iterator, and test cases for old '@@iterator' from tests in dom/.
Attachment #8554698 - Flags: review?(bugs)
Part 8 removes JS_HAS_SYMBOLS and ITERATOR_SYMBOL from Promise-backend.js.
Attachment #8554705 - Flags: review?(paolo.mozmail)
Attachment #8554698 - Flags: review?(bugs) → review+
Attachment #8554692 - Flags: review?(nfitzgerald) → review+
Attachment #8554687 - Flags: review?(bmcbride) → review+
Attachment #8554705 - Flags: review?(paolo.mozmail) → review+
Attachment #8554514 - Flags: review?(jwalden+bmo) → review+
Assignee: nobody → arai_a
Didn't notice this until it landed. :) Thanks!
Blocks: 1129756
You need to log in before you can comment on or make changes to this bug.