Closed
Bug 1069416
Opened 8 years ago
Closed 8 years ago
Strip out JS_HAS_SYMBOLS #ifdefs
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: jorendorff, Assigned: arai)
References
Details
Attachments
(8 files)
19.22 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
17.52 KB,
patch
|
evilpie
:
review+
mrbkap
:
review+
|
Details | Diff | Splinter Review |
79.22 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
4.00 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
1.75 KB,
patch
|
Unfocused
:
review+
|
Details | Diff | Splinter Review |
6.51 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
6.66 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
2.95 KB,
patch
|
Paolo
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•8 years ago
|
||
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
Assignee | ||
Comment 2•8 years ago
|
||
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 3•8 years ago
|
||
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+
Assignee | ||
Comment 4•8 years ago
|
||
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)
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8554514 -
Flags: review?(evilpies) → review?(jwalden+bmo)
Comment 7•8 years ago
|
||
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 8•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8554505 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 9•8 years ago
|
||
Part 5 removes JS_HAS_SYMBOLS and kIteratorSymbol from CustomizableUI.jsm.
Attachment #8554687 -
Flags: review?(bmcbride)
Assignee | ||
Comment 10•8 years ago
|
||
Part 6 removes JS_HAS_SYMBOLS and ITERATOR_SYMBOL from devtools.
Attachment #8554692 -
Flags: review?(nfitzgerald)
Assignee | ||
Comment 11•8 years ago
|
||
Part 7 removes JS_HAS_SYMBOLS, std_iterator, and test cases for old '@@iterator' from tests in dom/.
Attachment #8554698 -
Flags: review?(bugs)
Assignee | ||
Comment 12•8 years ago
|
||
Part 8 removes JS_HAS_SYMBOLS and ITERATOR_SYMBOL from Promise-backend.js.
Attachment #8554705 -
Flags: review?(paolo.mozmail)
Updated•8 years ago
|
Attachment #8554698 -
Flags: review?(bugs) → review+
Updated•8 years ago
|
Attachment #8554692 -
Flags: review?(nfitzgerald) → review+
Updated•8 years ago
|
Attachment #8554687 -
Flags: review?(bmcbride) → review+
Updated•8 years ago
|
Attachment #8554705 -
Flags: review?(paolo.mozmail) → review+
Updated•8 years ago
|
Attachment #8554514 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 13•8 years ago
|
||
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
Updated•8 years ago
|
Assignee: nobody → arai_a
Comment 14•8 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Reporter | ||
Comment 15•8 years ago
|
||
Didn't notice this until it landed. :) Thanks!
You need to log in
before you can comment on or make changes to this bug.
Description
•