Thanks for the detailed feedback; hopefully addressed.
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) \
On 2014/02/14 10:55:30, rossberg wrote:
Seems like this runtime function has become obsolete now.
I don't understand; it's called by ToObject() to create a wrapper
object.
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)) {
On 2014/02/14 10:55:30, rossberg wrote:
This should be subsumed by the default case below.
Yes. I noticed we were unnecessarily calling ToPrimitive() in two cases
involving symbol values, so added explicit checks. (This matters now
that Symbol wrappers throw on ToPrimitive().)
The spec hasn't changed here, so simply not correct as-was.
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]))
On 2014/02/14 10:55:30, rossberg wrote:
We should keep the equivalent of the previous tests, by using Object()
instead
of new Symbol().
Done.
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() })
On 2014/02/14 10:55:30, rossberg wrote:
We should check the actual results here, not just that they don't
throw anymore.
Done, carrying over the predicate from symbols.js
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/private.js#newcode153
test/mjsunit/harmony/private.js:153: assertDoesNotThrow(function() {
symbols[i].toString() })
On 2014/02/14 10:55:30, rossberg wrote:
Check result.
Done.
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/private.js#newcode167
test/mjsunit/harmony/private.js:167: assertDoesNotThrow(function() {
symbols[i].toString() })
On 2014/02/14 10:55:30, rossberg wrote:
Check result.
Done.
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]))
On 2014/02/14 10:55:30, rossberg wrote:
We should keep the original tests, by replacing "new Symbol()" with
"Object()".
Done.
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) {
On 2014/02/14 10:55:30, rossberg wrote:
Please move this down into TestToString.
I've kept it here, better to have it be next to where 'symbols' is
populated. (And it is now used outside TestToString().)
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/symbols.js#newcode54
test/mjsunit/harmony/symbols.js:54: symbols.push((indirect()))
On 2014/02/14 10:55:30, rossberg wrote:
Nit: redundant parens
ok
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/symbols.js#newcode121
test/mjsunit/harmony/symbols.js:121:
assertTrue(isValidSymbolString(stringValue))
On 2014/02/14 10:55:30, rossberg wrote:
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.
Removed
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]) })
On 2014/02/14 10:55:30, rossberg wrote:
Nit: line length
Tweaked, but isn't it < 80?
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/symbols.js#newcode201
test/mjsunit/harmony/symbols.js:201: assertDoesNotThrow(function() {
symbols[i].toString() })
On 2014/02/14 10:55:30, rossberg wrote:
Check result
Done, using isValidSymbolString().
https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/symbols.js#newcode215
test/mjsunit/harmony/symbols.js:215: assertDoesNotThrow(function() {
symbols[i].toString() })
On 2014/02/14 10:55:30, rossberg wrote:
Check result
Done.
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("") })
On 2014/02/14 10:55:30, rossberg wrote:
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).
Done.
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.
