https://codereview.chromium.org/118553003/diff/400001/src/symbol.js
File src/symbol.js (right):
https://codereview.chromium.org/118553003/diff/400001/src/symbol.js#newcode59
src/symbol.js:59: throw MakeTypeError('symbol_to_value', []);
We should leave this out for now (and also not do it in toString). I
consider this a spec bug, since it seems inconsistent with the rest of
the language. It also breaks our optimisation of not wrapping
primitives, as you seem to have observed (unless we move this file to
sloppy mode).
https://codereview.chromium.org/118553003/diff/400001/test/mjsunit/harmony/symbols.js
File test/mjsunit/harmony/symbols.js (right):
https://codereview.chromium.org/118553003/diff/400001/test/mjsunit/harmony/symbols.js#newcode38
test/mjsunit/harmony/symbols.js:38: function isValidSymbolString(s) {
I'd prefer to inline this where it's actually used.
https://codereview.chromium.org/118553003/diff/400001/test/mjsunit/harmony/symbols.js#newcode54
test/mjsunit/harmony/symbols.js:54: symbols.push((indirect()))
Nit: remove redundant parens
https://codereview.chromium.org/118553003/diff/400001/test/mjsunit/harmony/symbols.js#newcode106
test/mjsunit/harmony/symbols.js:106: assertThrows(function() {
Symbol.prototype.valueOf.call(symbols[i]) }, TypeError)
Nit: line length
https://codereview.chromium.org/118553003/diff/400001/test/mjsunit/harmony/symbols.js#newcode116
test/mjsunit/harmony/symbols.js:116: assertThrows(function() {
(Symbol(symbols[i])).toString() }, TypeError)
Nit: redundant parens. Also, it's the constructor that actually throws,
so this test seems out of place here.
https://codereview.chromium.org/118553003/diff/400001/test/mjsunit/harmony/symbols.js#newcode117
test/mjsunit/harmony/symbols.js:117: assertThrows(function() {
Symbol.prototype.toString.call(symbols[i]) }, TypeError)
Nit: line length
https://codereview.chromium.org/118553003/diff/400001/test/mjsunit/harmony/symbols.js#newcode230
test/mjsunit/harmony/symbols.js:230: Symbol.prototype.extra = function
(m) { return (m + this.toString()) }
Nit: move this inside the test function
Also, I'm not sure why to test toString here. Perhaps the function
should first assert that `this' is an object, and then return
this.valueOf for comparison.
Moreover, we should have an analogous test using a strict-mode method.
https://codereview.chromium.org/118553003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
