Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
118553003:
Upgrade Symbol implementation to match current ES6 behavior. (Closed)
|
Created:
6 years, 11 months ago by sof Modified:
6 years, 10 months ago CC:
v8-dev Base URL:
git://github.com/v8/v8.git@bleeding_edge Visibility:
Public. |
DescriptionUpgrade Symbol implementation to match current ES6 behavior.
Refresh the implementation of Symbols to catch up with what the
specification now mandates:
* The global Symbol() function manufactures new Symbol values,
optionally with a string description attached.
* Invoking Symbol() as a constructor will now throw.
* ToString() over Symbol values still throws, and
Object.prototype.toString() stringifies like before.
* A Symbol value is wrapped in a Symbol object either implicitly if
it is the receiver, or explicitly done via Object(symbolValue) or
(new Object(symbolValue).)
* The Symbol.prototype.toString() method no longer throws on Symbol
wrapper objects (nor Symbol values.) Ditto for Symbol.prototype.valueOf().
* Symbol.prototype.toString() stringifies as "Symbol("<description>"),
valueOf() returns the wrapper's Symbol value.
* ToPrimitive() over Symbol wrapper objects now throws.
Overall, this provides a stricter separation between Symbol values and
wrapper objects than before, and the explicit fetching out of the
description (nee name) via the "name" property is no longer supported
(by the spec nor the implementation.)
Adjusted existing Symbol test files to fit current, adding some extra
tests for new/changed behavior.
LOG=N
R=arv@chromium.org, rossberg@chromium.org, arv, rossberg
BUG=v8:3053
Committed: https://code.google.com/p/v8/source/detail?r=19490
Patch Set 1 #
Total comments: 43
Patch Set 2 : Fix fundamental issues surrounding Symbol values vs (wrapper) objects #
Total comments: 7
Patch Set 3 : Rebased + adapted valueOf/toString autoboxing to work post-CallICs. #Patch Set 4 : Remove uninteresting equality test #
Total comments: 8
Patch Set 5 : Allow valueOf()/toString() to be called over Symbol values. #
Total comments: 31
Patch Set 6 : EQUALS(): add missing Symbol checks #
Total comments: 9
Patch Set 7 : Some test improvements. #
Messages
Total messages: 35 (0 generated)
Please take a look when you next have a moment. The encoding/representation of the Symbol wrapper objects is one part I feel so-so content with. A simple enough implementation of them, but there might well be better alternatives.
Hmmmm... I'm really confused about this change. I only looked at the diffs to the test cases so far, but many of them don't seem to match what I read in the (new) spec. I also don't understand why we need to handle the symbol wrapper object different than before, and different from other wrappers. (If the spec really makes that necessary then I think the spec should be changed.) https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols.js File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:75: assertSame(Function.prototype, Symbol.prototype) That seems wrong. Symbol.prototype is an ordinary object. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:78: assertSame(symbolPrototype, Object(Symbol()).__proto__) Is this different from Symbol.prototype? Why? https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:79: assertSame(Symbol.prototype, Symbol.__proto__) Shouldn't Symbol.__proto__ be Function.prototype? https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:81: assertSame(Function.prototype, (Symbol()).__proto__) And Symbol().__proto__ should be Symbol.prototype https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:82: assertTrue(typeof ((Symbol()).prototype) === "undefined") Not sure why this test is necessary. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:91: assertFalse(Object === Symbol.prototype.constructor) This should be true now. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:93: assertSame(Function, Symbol.prototype.constructor) This should be Object, not Function https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:94: assertSame(Function, Symbol().__proto__.constructor) Dito https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:95: assertSame(Function, Object(Symbol()).valueOf().__proto__.constructor) Dito here. And why the redundant valueOf()? https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:96: assertSame(Function, (Symbol).__proto__.constructor) This is an unrelated test, since it tests the constructor of the constructor. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:97: assertSame(Function, (Symbol()).__proto__.constructor) This is repeating line 94 https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:98: assertSame(Symbol, (Object(Symbol())).__proto__.constructor) Nit: redundant parens (here and above) https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:101: assertSame(Function, symbols[i].__proto__.constructor) Should also be Symbol https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:109: var value = Object(symbols[i]).valueOf() Why the redundant Object()? https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:119: assertDoesNotThrow(function () { stringified = String(Object(symbols[i])); }) Nit: line length (just remove the space after 'function') https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:125: assertDoesNotThrow(function() { Object(symbols[i]).toString() }) Why not check the expected result? https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:166: assertTrue(symbols[i] === Object(symbols[i]).valueOf()) The original test was to ensure that symbols are distinct from their wrappers, so this should be assertFalse(symbols[i] === Object(symbols[i])) https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:167: assertTrue(Object(symbols[i]).valueOf() === symbols[i]) Likewise https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:170: assertEquals(Object(symbols[i]), Object(symbols[i])) This is surprising. Why are the two objects equal? https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:175: assertFalse(sym1 == Object(symbols[i])) That seems to contradict line 170 -- I'm really confused now. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:205: assertEquals(symbols[i], Object(symbols[i]).valueOf()) Why is Object() needed? https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:220: assertEquals(symbols[i], Object(symbols[i]).valueOf()) Dito
Commented on some of them; I think we're not in sync here somehow on spec reading. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols.js File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:75: assertSame(Function.prototype, Symbol.prototype) On 2014/01/08 17:52:25, rossberg wrote: > That seems wrong. Symbol.prototype is an ordinary object. https://people.mozilla.org/~jorendorff/es6-draft.html#sec-properties-of-the-s... says it's Function.prototype. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:78: assertSame(symbolPrototype, Object(Symbol()).__proto__) On 2014/01/08 17:52:25, rossberg wrote: > Is this different from Symbol.prototype? Why? Yes, Object(Symbol()).__proto__ is the prototype of the wrapper object, not Symbol.prototype. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:81: assertSame(Function.prototype, (Symbol()).__proto__) On 2014/01/08 17:52:25, rossberg wrote: > And Symbol().__proto__ should be Symbol.prototype Definitely not. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:82: assertTrue(typeof ((Symbol()).prototype) === "undefined") On 2014/01/08 17:52:25, rossberg wrote: > Not sure why this test is necessary. Just testing that typeof over this .prototype is behaving like it does for other primitive values. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:91: assertFalse(Object === Symbol.prototype.constructor) On 2014/01/08 17:52:25, rossberg wrote: > This should be true now. No, I believe this is correct. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:93: assertSame(Function, Symbol.prototype.constructor) On 2014/01/08 17:52:25, rossberg wrote: > This should be Object, not Function Same; are we reading the same draft version of the spec? https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:205: assertEquals(symbols[i], Object(symbols[i]).valueOf()) On 2014/01/08 17:52:25, rossberg wrote: > Why is Object() needed? To test that the wrapper object's valueOf() returns the same symbol value.
This does not look correct to me. Symbol should behave like Number, String and Boolean with the exception of Symbol.prototype[Symbol.toPrimitive] https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols.js File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:75: assertSame(Function.prototype, Symbol.prototype) On 2014/01/08 18:13:43, sof wrote: > On 2014/01/08 17:52:25, rossberg wrote: > > That seems wrong. Symbol.prototype is an ordinary object. > > https://people.mozilla.org/%7Ejorendorff/es6-draft.html#sec-properties-of-the... > says it's Function.prototype. It says that "The value of the [[Prototype]] internal slot" is Function.prototype. This is reflected as assertSame(Object.getPrototypeOf(Symbol), Function.prototype) https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:78: assertSame(symbolPrototype, Object(Symbol()).__proto__) On 2014/01/08 18:13:43, sof wrote: > On 2014/01/08 17:52:25, rossberg wrote: > > Is this different from Symbol.prototype? Why? > > Yes, Object(Symbol()).__proto__ is the prototype of the wrapper object, not > Symbol.prototype. The [[Prototype]] of a symbol wrapper should be Symbol.prototype. This is the same as for number, string and boolean wrappers. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:81: assertSame(Function.prototype, (Symbol()).__proto__) On 2014/01/08 18:13:43, sof wrote: > On 2014/01/08 17:52:25, rossberg wrote: > > And Symbol().__proto__ should be Symbol.prototype > > Definitely not. When you do a [[Get]] on a non object a temporary wrapper is created so var sym = Symbol(); sym.foo === Object(sym).foo; for all foos https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:91: assertFalse(Object === Symbol.prototype.constructor) On 2014/01/08 18:13:43, sof wrote: > On 2014/01/08 17:52:25, rossberg wrote: > > This should be true now. > > No, I believe this is correct. Looks correct to me too: assertSame(Symbol.prototype.constructor, Symbol); just like for all other wrappers. However, using assertSame would be better. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:93: assertSame(Function, Symbol.prototype.constructor) On 2014/01/08 18:13:43, sof wrote: > On 2014/01/08 17:52:25, rossberg wrote: > > This should be Object, not Function > > Same; are we reading the same draft version of the spec? This should be assertSame(Symbol.prototype.constructor, Symbol); https://people.mozilla.org/~jorendorff/es6-draft.html#sec-symbol.prototype.co... https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:94: assertSame(Function, Symbol().__proto__.constructor) On 2014/01/08 17:52:25, rossberg wrote: > Dito Should be assertSame(Symbol, Symbol().__proto__.constructor)
On 2014/01/08 18:54:43, arv wrote: > This does not look correct to me. Symbol should behave like Number, String and > Boolean with the exception of Symbol.prototype[Symbol.toPrimitive] > To quote from 19.4.1 https://people.mozilla.org/~jorendorff/es6-draft.html#sec-symbol-constructor "The Symbol constructor is the %Symbol% intrinsic object and the initial value of the Symbol property of the global object." Symbol doesn't refer to Symbol the primitive type, but the Symbol constructor. That's the reading this CL is based on.
On 2014/01/08 18:58:01, sof wrote: > On 2014/01/08 18:54:43, arv wrote: > > This does not look correct to me. Symbol should behave like Number, String and > > Boolean with the exception of Symbol.prototype[Symbol.toPrimitive] > > > > To quote from 19.4.1 > > https://people.mozilla.org/%7Ejorendorff/es6-draft.html#sec-symbol-constructor > > "The Symbol constructor is the %Symbol% intrinsic object and the initial value > of the Symbol property of the global object." > > Symbol doesn't refer to Symbol the primitive type, but the Symbol constructor. > That's the reading this CL is based on. Yes, the global Symbol is a Function object (constructor). Just like the global Number is a Function object.
https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols.js File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:75: assertSame(Function.prototype, Symbol.prototype) On 2014/01/08 18:13:43, sof wrote: > On 2014/01/08 17:52:25, rossberg wrote: > > That seems wrong. Symbol.prototype is an ordinary object. > > https://people.mozilla.org/%7Ejorendorff/es6-draft.html#sec-properties-of-the... > says it's Function.prototype. Don't confuse .prototype and .[[Prototype]] (a.k.a. __proto__). As Arv says, that spec section says that Symbol.__proto__ === Function.prototype. Symbol.prototype is specified elsewhere: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-properties-of-the-sy... https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:81: assertSame(Function.prototype, (Symbol()).__proto__) On 2014/01/08 18:13:43, sof wrote: > On 2014/01/08 17:52:25, rossberg wrote: > > And Symbol().__proto__ should be Symbol.prototype > > Definitely not. See above. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:82: assertTrue(typeof ((Symbol()).prototype) === "undefined") On 2014/01/08 18:13:43, sof wrote: > On 2014/01/08 17:52:25, rossberg wrote: > > Not sure why this test is necessary. > > Just testing that typeof over this .prototype is behaving like it does for other > primitive values. Well, it just tests that the symbol wrapper object does not have a property named 'prototype'. It's not wrong to assert this, but I don't see a particular reason to do so -- that property name is only special for function objects. For others, it's no more relevant than testing that 'xyz' is undefined. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:91: assertFalse(Object === Symbol.prototype.constructor) On 2014/01/08 18:54:44, arv wrote: > On 2014/01/08 18:13:43, sof wrote: > > On 2014/01/08 17:52:25, rossberg wrote: > > > This should be true now. > > > > No, I believe this is correct. > > Looks correct to me too: > > assertSame(Symbol.prototype.constructor, Symbol); > > just like for all other wrappers. > > However, using assertSame would be better. Oops, you are both right, sorry. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:93: assertSame(Function, Symbol.prototype.constructor) On 2014/01/08 18:54:44, arv wrote: > On 2014/01/08 18:13:43, sof wrote: > > On 2014/01/08 17:52:25, rossberg wrote: > > > This should be Object, not Function > > > > Same; are we reading the same draft version of the spec? > > This should be > > assertSame(Symbol.prototype.constructor, Symbol); > > https://people.mozilla.org/%7Ejorendorff/es6-draft.html#sec-symbol.prototype.... Arv is correct. https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:205: assertEquals(symbols[i], Object(symbols[i]).valueOf()) On 2014/01/08 18:13:43, sof wrote: > On 2014/01/08 17:52:25, rossberg wrote: > > Why is Object() needed? > > To test that the wrapper object's valueOf() returns the same symbol value. But the wrapper is supposed to be created implicitly when you project from a primitive, you don't need to invoke Object() explicitly.
https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols.js File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:75: assertSame(Function.prototype, Symbol.prototype) On 2014/01/09 10:35:53, rossberg wrote: > On 2014/01/08 18:13:43, sof wrote: > > On 2014/01/08 17:52:25, rossberg wrote: > > > That seems wrong. Symbol.prototype is an ordinary object. > > > > > https://people.mozilla.org/%257Ejorendorff/es6-draft.html#sec-properties-of-t... > > says it's Function.prototype. > > Don't confuse .prototype and .[[Prototype]] (a.k.a. __proto__). As Arv says, > that spec section says that Symbol.__proto__ === Function.prototype. > > Symbol.prototype is specified elsewhere: > > http://people.mozilla.org/%7Ejorendorff/es6-draft.html#sec-properties-of-the-... I made that only available on the "SymbolWrapper" object that Object() instantiates. I will need to sit down and change the encoding of Symbol to expose the expected .prototype, updating the tests here to reflect this. Sorry about the mess.
On 2014/01/09 13:17:12, sof wrote: > I will need to sit down and change the encoding of Symbol to expose the expected > .prototype, updating the tests here to reflect this. Sorry about the mess. Good thing is that that should vastly simplify this change. AFAICS, most of the spec changes should be little more than one liners to implement.
https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols.js File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... test/mjsunit/harmony/symbols.js:205: assertEquals(symbols[i], Object(symbols[i]).valueOf()) On 2014/01/09 10:35:53, rossberg wrote: > On 2014/01/08 18:13:43, sof wrote: > > On 2014/01/08 17:52:25, rossberg wrote: > > > Why is Object() needed? > > > > To test that the wrapper object's valueOf() returns the same symbol value. > > But the wrapper is supposed to be created implicitly when you project from a > primitive, you don't need to invoke Object() explicitly. Where is that requirement given? Symbol.prototype.valueOf() states that if there's no [[SymbolData]] on its incoming 'this' (== not a wrapper object), it should throw.
On 2014/01/10 10:53:52, sof wrote: > https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols.js > File test/mjsunit/harmony/symbols.js (right): > > https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... > test/mjsunit/harmony/symbols.js:205: assertEquals(symbols[i], > Object(symbols[i]).valueOf()) > On 2014/01/09 10:35:53, rossberg wrote: > > On 2014/01/08 18:13:43, sof wrote: > > > On 2014/01/08 17:52:25, rossberg wrote: > > > > Why is Object() needed? > > > > > > To test that the wrapper object's valueOf() returns the same symbol value. > > > > But the wrapper is supposed to be created implicitly when you project from a > > primitive, you don't need to invoke Object() explicitly. > > Where is that requirement given? Symbol.prototype.valueOf() states that if > there's no [[SymbolData]] on its incoming 'this' (== not a wrapper object), it > should throw. Implicitly creating wrapper objects is the generic semantics of property access on primitive values in JavaScript. See the definition of GetValue: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-getvalue which is invoked whenever a "reference" is forced, such as the one returned by property access: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-property-accessors-r... Common patterns like string.length or number.toString() rely on this mechanism.
On 2014/01/10 11:32:30, rossberg wrote: > On 2014/01/10 10:53:52, sof wrote: > > > https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols.js > > File test/mjsunit/harmony/symbols.js (right): > > > > > https://codereview.chromium.org/118553003/diff/1/test/mjsunit/harmony/symbols... > > test/mjsunit/harmony/symbols.js:205: assertEquals(symbols[i], > > Object(symbols[i]).valueOf()) > > On 2014/01/09 10:35:53, rossberg wrote: > > > On 2014/01/08 18:13:43, sof wrote: > > > > On 2014/01/08 17:52:25, rossberg wrote: > > > > > Why is Object() needed? > > > > > > > > To test that the wrapper object's valueOf() returns the same symbol value. > > > > > > But the wrapper is supposed to be created implicitly when you project from a > > > primitive, you don't need to invoke Object() explicitly. > > > > Where is that requirement given? Symbol.prototype.valueOf() states that if > > there's no [[SymbolData]] on its incoming 'this' (== not a wrapper object), it > > should throw. > > Implicitly creating wrapper objects is the generic semantics of property access > on primitive values in JavaScript. See the definition of GetValue: > > http://people.mozilla.org/%7Ejorendorff/es6-draft.html#sec-getvalue > > which is invoked whenever a "reference" is forced, such as the one returned by > property access: > > > http://people.mozilla.org/%7Ejorendorff/es6-draft.html#sec-property-accessors... > > Common patterns like string.length or number.toString() rely on this mechanism. Thanks for clarifying. The stratification of Symbol values vs wrappers appeared stronger when I read the discussion surrounding the spec update, but not so. To address, I had to disable the optimization that passes non-boxed values to Symbol builtins.
With TC39 meeting having come & gone (?), might this possibly be looked at again at some point sooner? :)
I think there are still some misunderstandings. I think the equality is wrong. https://codereview.chromium.org/118553003/diff/170001/src/messages.js File src/messages.js (right): https://codereview.chromium.org/118553003/diff/170001/src/messages.js#newcode184 src/messages.js:184: symbol_to_string: ["Cannot convert Symbol values to strings"], "value to string" maybe? https://codereview.chromium.org/118553003/diff/170001/src/messages.js#newcode186 src/messages.js:186: symbol_to_primitive: ["Cannot convert Symbol wrapper objects to primitive values"], same? https://codereview.chromium.org/118553003/diff/170001/test/mjsunit/harmony/sy... File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/170001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:45: function IndirectSymbol() { return Symbol() } Should be lower case now since it is not a constructor. https://codereview.chromium.org/118553003/diff/170001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:172: assertEquals(Object(symbols[i]), Object(symbols[i])) This looks strange. If symbol[i] is a symbol (not a symbol wrapper) then this should be false. Are all elements in symbols wrappers? It does not seem like that is the case on line 49.
https://codereview.chromium.org/118553003/diff/170001/test/mjsunit/harmony/sy... File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/170001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:172: assertEquals(Object(symbols[i]), Object(symbols[i])) On 2014/02/01 01:33:10, arv wrote: > This looks strange. If symbol[i] is a symbol (not a symbol wrapper) then this > should be false. Are all elements in symbols wrappers? It does not seem like > that is the case on line 49. They're all Symbol values. This is an equals check, not same, so why should deepObjectEquals() return false? (Perhaps not a very interesting to test to add, I can remove.)
The removal of CallICs that just landed voided the autoboxing of Symbol values needed here. Trying to get the code generator to emit the necessary wrapping of the receiver when compiling valueOf() or toString() calls, but not entirely obvious.
https://codereview.chromium.org/118553003/diff/170001/test/mjsunit/harmony/sy... File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/170001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:172: assertEquals(Object(symbols[i]), Object(symbols[i])) On 2014/02/01 13:58:10, sof wrote: > On 2014/02/01 01:33:10, arv wrote: > > This looks strange. If symbol[i] is a symbol (not a symbol wrapper) then this > > should be false. Are all elements in symbols wrappers? It does not seem like > > that is the case on line 49. > > They're all Symbol values. This is an equals check, not same, so why should > deepObjectEquals() return false? (Perhaps not a very interesting to test to add, > I can remove.) If all elements in symbols are symbol values (not symbol wrapper objects) this should be assertNotEquals since a new wrapper is always created when you do Object(value).
https://codereview.chromium.org/118553003/diff/170001/test/mjsunit/harmony/sy... File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/170001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:172: assertEquals(Object(symbols[i]), Object(symbols[i])) On 2014/02/02 19:49:35, arv wrote: > On 2014/02/01 13:58:10, sof wrote: > > On 2014/02/01 01:33:10, arv wrote: > > > This looks strange. If symbol[i] is a symbol (not a symbol wrapper) then > this > > > should be false. Are all elements in symbols wrappers? It does not seem like > > > that is the case on line 49. > > > > They're all Symbol values. This is an equals check, not same, so why should > > deepObjectEquals() return false? (Perhaps not a very interesting to test to > add, > > I can remove.) > > If all elements in symbols are symbol values (not symbol wrapper objects) this > should be assertNotEquals since a new wrapper is always created when you do > Object(value). I know; the object also doesn't have any "keys" in the (Object.keys() sense), so I don't think it much of a difference how you want to phrase that equality check for these wrapper objects. Had assertNotEquals existed, that is. The offer to remove the test still stands; I don't think there is anything fundamentally wrong here in the interpretation of symbol values vs wrappers.
On 2014/02/02 19:59:56, sof wrote: > https://codereview.chromium.org/118553003/diff/170001/test/mjsunit/harmony/sy... > File test/mjsunit/harmony/symbols.js (right): > > https://codereview.chromium.org/118553003/diff/170001/test/mjsunit/harmony/sy... > test/mjsunit/harmony/symbols.js:172: assertEquals(Object(symbols[i]), > Object(symbols[i])) > On 2014/02/02 19:49:35, arv wrote: > > On 2014/02/01 13:58:10, sof wrote: > > > On 2014/02/01 01:33:10, arv wrote: > > > > This looks strange. If symbol[i] is a symbol (not a symbol wrapper) then > > this > > > > should be false. Are all elements in symbols wrappers? It does not seem > like > > > > that is the case on line 49. > > > > > > They're all Symbol values. This is an equals check, not same, so why should > > > deepObjectEquals() return false? (Perhaps not a very interesting to test to > > add, > > > I can remove.) > > > > If all elements in symbols are symbol values (not symbol wrapper objects) this > > should be assertNotEquals since a new wrapper is always created when you do > > Object(value). > > I know; the object also doesn't have any "keys" in the (Object.keys() sense), so > I don't think it much of a difference how you want to phrase that equality check > for these wrapper objects. Had assertNotEquals existed, that is. > > The offer to remove the test still stands; I don't think there is anything > fundamentally wrong here in the interpretation of symbol values vs wrappers. I see, assertEquals is just broken. For other wrappers it unwraps which is clearly wrong. It also trats two completely different object as equal. Yikes. Can you change the test to? assertTrue(Object(symbol[i]) !== Object(symbol[i])); or add assertNotSame and use that?
On 2014/02/02 20:08:59, arv wrote: > On 2014/02/02 19:59:56, sof wrote: > > > https://codereview.chromium.org/118553003/diff/170001/test/mjsunit/harmony/sy... > > File test/mjsunit/harmony/symbols.js (right): > > > > > https://codereview.chromium.org/118553003/diff/170001/test/mjsunit/harmony/sy... > > test/mjsunit/harmony/symbols.js:172: assertEquals(Object(symbols[i]), > > Object(symbols[i])) > > On 2014/02/02 19:49:35, arv wrote: > > > On 2014/02/01 13:58:10, sof wrote: > > > > On 2014/02/01 01:33:10, arv wrote: > > > > > This looks strange. If symbol[i] is a symbol (not a symbol wrapper) then > > > this > > > > > should be false. Are all elements in symbols wrappers? It does not seem > > like > > > > > that is the case on line 49. > > > > > > > > They're all Symbol values. This is an equals check, not same, so why > should > > > > deepObjectEquals() return false? (Perhaps not a very interesting to test > to > > > add, > > > > I can remove.) > > > > > > If all elements in symbols are symbol values (not symbol wrapper objects) > this > > > should be assertNotEquals since a new wrapper is always created when you do > > > Object(value). > > > > I know; the object also doesn't have any "keys" in the (Object.keys() sense), > so > > I don't think it much of a difference how you want to phrase that equality > check > > for these wrapper objects. Had assertNotEquals existed, that is. > > > > The offer to remove the test still stands; I don't think there is anything > > fundamentally wrong here in the interpretation of symbol values vs wrappers. > > I see, assertEquals is just broken. For other wrappers it unwraps which is > clearly wrong. It also trats two completely different object as equal. Yikes. > > Can you change the test to? > > assertTrue(Object(symbol[i]) !== Object(symbol[i])); > > or add assertNotSame and use that? I added a (!==) variant (but kept the assertEquals() version..will tidy that up tomorrow.) I don't like to leave stuff in a broken state, so I added some autoboxing of Symbol value receivers to the x64 and ia32 code generators, to have this CL work again wrt HEAD. Along with FIXMEs; I hope someone can give me a nudge in a better direction on how to go about this more properly.
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/sy... File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/400001/test/mjsunit/harmony/sy... 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/sy... test/mjsunit/harmony/symbols.js:54: symbols.push((indirect())) Nit: remove redundant parens https://codereview.chromium.org/118553003/diff/400001/test/mjsunit/harmony/sy... 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/sy... 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/sy... 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/sy... 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/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', []); On 2014/02/04 15:16:25, rossberg wrote: > 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). Not just non-strict, but the methods would also have to be marked as non-native. What has changed here since e.g., https://mail.mozilla.org/pipermail/es-discuss/2013-October/033876.html Defining away what was the main part of this CL, is a suboptimal situation (for me.) But OK.
On 2014/02/04 15:40:58, sof wrote: > 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', []); > On 2014/02/04 15:16:25, rossberg wrote: > > 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). > > Not just non-strict, but the methods would also have to be marked as non-native. Hm, why? > What has changed here since e.g., > > https://mail.mozilla.org/pipermail/es-discuss/2013-October/033876.html Well, Allen does not mention throwing on unwrapped symbols there -- which is why I actually was somewhat surprised to see it in the spec. > Defining away what was the main part of this CL, is a suboptimal situation (for > me.) But OK. Sorry about that. :(
On 2014/02/04 15:58:31, rossberg wrote: > > Defining away what was the main part of this CL, is a suboptimal situation > (for > > me.) But OK. > > Sorry about that. :( But in any case, it's just one item from your list, isn't it?
Filed a bug: https://bugs.ecmascript.org/show_bug.cgi?id=2498
On 2014/02/04 16:09:00, rossberg wrote: > Filed a bug: > > https://bugs.ecmascript.org/show_bug.cgi?id=2498 Thanks, I'll await the outcome of that. Continuing here before it is settled doesn't make too much sense. (Especially, if I understand you correctly, you no longer want toString() to throw over Symbol values.)
On 2014/02/05 08:29:29, sof wrote: > On 2014/02/04 16:09:00, rossberg wrote: > > Filed a bug: > > > > https://bugs.ecmascript.org/show_bug.cgi?id=2498 > > Thanks, I'll await the outcome of that. Continuing here before it is settled > doesn't make too much sense. Allen just announced that he has changed the spec respectively: https://mail.mozilla.org/pipermail/es-discuss/2014-February/036263.html He hasn't closed the bug yet, but I think it is safe to proceed anyway. > (Especially, if I understand you correctly, you no longer want toString() to > throw over Symbol values.) Yes, both toString and valueOf should work just fine for unwrapped symbols. Everything else in this CL remains (although the former should enable you to remove some of the extra complexity).
On 2014/02/12 10:42:49, rossberg wrote: > On 2014/02/05 08:29:29, sof wrote: > > On 2014/02/04 16:09:00, rossberg wrote: > > > Filed a bug: > > > > > > https://bugs.ecmascript.org/show_bug.cgi?id=2498 > > > > Thanks, I'll await the outcome of that. Continuing here before it is settled > > doesn't make too much sense. > > Allen just announced that he has changed the spec respectively: > > https://mail.mozilla.org/pipermail/es-discuss/2014-February/036263.html > > He hasn't closed the bug yet, but I think it is safe to proceed anyway. > Yes, thanks - I noticed that. He put together a good writeup of the reasoning behind the design also, https://mail.mozilla.org/pipermail/es-discuss/2014-February/036261.html
On 2014/02/12 10:42:49, rossberg wrote: .. > > Yes, both toString and valueOf should work just fine for unwrapped symbols. > Everything else in this CL remains (although the former should enable you to > remove some of the extra complexity). Now (finally) done.
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/pr... File test/mjsunit/harmony/private.js (left): https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/pr... 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/pr... File test/mjsunit/harmony/private.js (right): https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/pr... 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/pr... 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/pr... test/mjsunit/harmony/private.js:153: assertDoesNotThrow(function() { symbols[i].toString() }) Check result. https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/pr... test/mjsunit/harmony/private.js:167: assertDoesNotThrow(function() { symbols[i].toString() }) Check result. https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/sy... File test/mjsunit/harmony/symbols.js (left): https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/sy... 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/sy... File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/sy... 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/sy... test/mjsunit/harmony/symbols.js:54: symbols.push((indirect())) Nit: redundant parens https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/sy... 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/sy... 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/sy... test/mjsunit/harmony/symbols.js:201: assertDoesNotThrow(function() { symbols[i].toString() }) Check result https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:215: assertDoesNotThrow(function() { symbols[i].toString() }) Check result https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/sy... 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).
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/pr... File test/mjsunit/harmony/private.js (left): https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/pr... 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/pr... File test/mjsunit/harmony/private.js (right): https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/pr... 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/pr... 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/pr... 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/sy... File test/mjsunit/harmony/symbols.js (left): https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/sy... 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/sy... File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/sy... 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/sy... 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/sy... 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/sy... 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/sy... 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/sy... 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/sy... 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.
LGTM Thanks for taking care of this. https://codereview.chromium.org/118553003/diff/620001/test/mjsunit/harmony/sy... File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/620001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:55: assertThrows(function () { Symbol(Symbol()) }, TypeError) These two assertThrows can be done once outside the outer loop. No need to test them 10 times each. https://codereview.chromium.org/118553003/diff/620001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:116: assertThrows(function() { (Symbol(symbols[i])).toString() }, TypeError) Doesn't this one throw due to Symbol(symbols[i]) and not due to the toString? https://codereview.chromium.org/118553003/diff/620001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:171: assertTrue(Object(symbols[i]) !== Object(symbols[i])) assertFalse(Object(symbols[i]) === Object(symbols[i])) for consistency? https://codereview.chromium.org/118553003/diff/620001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:229: Symbol.prototype.getThisProto = function () { return Object.getPrototypeOf(this); } long line https://codereview.chromium.org/118553003/diff/620001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:229: Symbol.prototype.getThisProto = function () { return Object.getPrototypeOf(this); } too many spaces. But after line wrapping it should be. Symbol.prototype.getThisProto = function() { return Object.getPrototypeOf(this) }
https://codereview.chromium.org/118553003/diff/620001/test/mjsunit/harmony/sy... File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/620001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:55: assertThrows(function () { Symbol(Symbol()) }, TypeError) On 2014/02/14 15:30:11, arv wrote: > These two assertThrows can be done once outside the outer loop. No need to test > them 10 times each. Yes. Belongs in TestNew() though, I think, so moved down a bit. https://codereview.chromium.org/118553003/diff/620001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:116: assertThrows(function() { (Symbol(symbols[i])).toString() }, TypeError) On 2014/02/14 15:30:11, arv wrote: > Doesn't this one throw due to Symbol(symbols[i]) and not due to the toString? Yes, it's not testing toString(); dropped. https://codereview.chromium.org/118553003/diff/620001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:171: assertTrue(Object(symbols[i]) !== Object(symbols[i])) On 2014/02/14 15:30:11, arv wrote: > assertFalse(Object(symbols[i]) === Object(symbols[i])) > > for consistency? Done. https://codereview.chromium.org/118553003/diff/620001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:229: Symbol.prototype.getThisProto = function () { return Object.getPrototypeOf(this); } On 2014/02/14 15:30:11, arv wrote: > long line Done.
LGTM, will land tomorrow 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 14:20:02, sof wrote: > 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. Ah, sorry, I missed that. But can't we use Object(symbol) in that place? Or would that be circular somehow? https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/sy... File test/mjsunit/harmony/symbols.js (right): https://codereview.chromium.org/118553003/diff/550001/test/mjsunit/harmony/sy... test/mjsunit/harmony/symbols.js:126: checkToString(function() { return Symbol.prototype.toString.call(symbols[i]) }) On 2014/02/14 14:20:02, sof wrote: > On 2014/02/14 10:55:30, rossberg wrote: > > Nit: line length > > Tweaked, but isn't it < 80? Looked like 83 in the code review...
Message was sent while issue was closed.
Committed patchset #7 manually as r19490 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
