Great, thanks for all the work. Mostly cosmetics remaining.
https://codereview.chromium.org/118553003/diff/550001/src/runtime.h
File src/runtime.h (right):
https://codereview.chromium.org/118553003/diff/550001/src/runtime.h#newcode317
src/runtime.h:317: F(NewSymbolWrapper, 1, 1) \
Seems like this runtime function has become obsolete now.
https://codereview.chromium.org/118553003/diff/550001/src/runtime.js
File src/runtime.js (right):
https://codereview.chromium.org/118553003/diff/550001/src/runtime.js#newcode83
src/runtime.js:83: } else if (IS_SYMBOL_WRAPPER(x) &&
IS_SYMBOL_WRAPPER(y)) {
This should be subsumed by the default case below.
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/private.js
File test/mjsunit/harmony/private.js (left):
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/private.js#oldcode131
test/mjsunit/harmony/private.js:131: assertFalse(symbols[i] === new
Symbol(symbols[i]))
We should keep the equivalent of the previous tests, by using Object()
instead of new Symbol().
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/private.js
File test/mjsunit/harmony/private.js (right):
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/private.js#newcode79
test/mjsunit/harmony/private.js:79: assertDoesNotThrow(function() {
symbols[i].toString() })
We should check the actual results here, not just that they don't throw
anymore.
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/private.js#newcode81
test/mjsunit/harmony/private.js:81: assertDoesNotThrow(function() {
Symbol.prototype.toString.call(symbols[i]) })
Nit: line length
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/private.js#newcode153
test/mjsunit/harmony/private.js:153: assertDoesNotThrow(function() {
symbols[i].toString() })
Check result.
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/private.js#newcode167
test/mjsunit/harmony/private.js:167: assertDoesNotThrow(function() {
symbols[i].toString() })
Check result.
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/symbols.js
File test/mjsunit/harmony/symbols.js (left):
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/symbols.js#oldcode159
test/mjsunit/harmony/symbols.js:159: assertFalse(symbols[i] === new
Symbol(symbols[i]))
We should keep the original tests, by replacing "new Symbol()" with
"Object()".
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/symbols.js
File test/mjsunit/harmony/symbols.js (right):
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/symbols.js#newcode38
test/mjsunit/harmony/symbols.js:38: function isValidSymbolString(s) {
Please move this down into TestToString.
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/symbols.js#newcode54
test/mjsunit/harmony/symbols.js:54: symbols.push((indirect()))
Nit: redundant parens
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/symbols.js#newcode121
test/mjsunit/harmony/symbols.js:121:
assertTrue(isValidSymbolString(stringValue))
I don't think we need the explicit assertDoesNotThrow here -- if it
throws, the test fails anyway. By getting rid of it, you can remove the
whole checkToString function, it seems to me.
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/symbols.js#newcode126
test/mjsunit/harmony/symbols.js:126: checkToString(function() { return
Symbol.prototype.toString.call(symbols[i]) })
Nit: line length
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/symbols.js#newcode201
test/mjsunit/harmony/symbols.js:201: assertDoesNotThrow(function() {
symbols[i].toString() })
Check result
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/symbols.js#newcode215
test/mjsunit/harmony/symbols.js:215: assertDoesNotThrow(function() {
symbols[i].toString() })
Check result
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/symbols.js#newcode234
test/mjsunit/harmony/symbols.js:234: assertDoesNotThrow(function() {
stringValue = symbols[i].extra("") })
Like above, you can remove the assertDoesNotThrow here to simplify the
code. Also, remove the dependency on symbol-to-string conversion by
choosing a simpler function for extra (returning this would probably be
good enough).
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.
