LGTM with comments addressed.
http://codereview.chromium.org/6335010/diff/6002/src/ast.cc File src/ast.cc (right): http://codereview.chromium.org/6335010/diff/6002/src/ast.cc#newcode245 src/ast.cc:245: if (handle->ToArrayIndex(&index)) { I already committed a patch for this case. During that I checked what happens, and this change isn't even sufficient. The ToArrayIndex function only works on numbers (Smi and HeapNumber), not strings. You need a second case inside the IsSymbol case to account for a string that represents an array index. http://codereview.chromium.org/6335010/diff/6002/src/parser.cc File src/parser.cc (right): http://codereview.chromium.org/6335010/diff/6002/src/parser.cc#newcode3077 src/parser.cc:3077: if (handle->ToArrayIndex(&hash)) { This still needs to handle array-index symbols. You can see the code in my change for ast.cc: http://codereview.chromium.org/6304016/ http://codereview.chromium.org/6335010/diff/6002/test/mjsunit/strict-mode.js File test/mjsunit/strict-mode.js (right): http://codereview.chromium.org/6335010/diff/6002/test/mjsunit/strict-mode.js#newcode125 test/mjsunit/strict-mode.js:125: For good measure, try with var x = {123: 1, "0123": 2}; var x = {123: 1, 123.00000000000000000000000000000000000000000000000000000000000000000001 : 2} and var x = {123: 1, "123.00000000000000000000000000000000000000000000000000000000000000000001" : 2} The first shouldn't throw, the second should (it's the same number), and the third shouldn't. http://codereview.chromium.org/6335010/diff/6002/test/mjsunit/strict-mode.js#newcode125 test/mjsunit/strict-mode.js:125: Also try get/set properties using strings and numbers, e.g.: var x = {get 12(){}, get "12"(){}}; var x = {get foo(){}, get "foo"(){}}; http://codereview.chromium.org/6335010/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
