LGTM
http://codereview.chromium.org/8341021/diff/1/src/messages.js File src/messages.js (right): http://codereview.chromium.org/8341021/diff/1/src/messages.js#newcode1157 src/messages.js:1157: var type = error.type; The spec doesn't say anything about reading .type. If that's something we use internally, we should first check (somehow) whether it is an internal error (i.e., we control .type) before reading or using it, so we don't trigger a user-visible action (e.g., a getter on "type"). http://codereview.chromium.org/8341021/diff/1/src/messages.js#newcode1158 src/messages.js:1158: var name = error.name According to the spec, we should do ToString on name before reading message. http://codereview.chromium.org/8341021/diff/1/src/messages.js#newcode1180 src/messages.js:1180: // the builtins object do not work inside of a catch clause. We should check whether this is really (still) the case. http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-string.js File test/mjsunit/cyclic-error-to-string.js (right): http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-string.js#newcode32 test/mjsunit/cyclic-error-to-string.js:32: assertEquals('Error', e + ''); I'd use String(e) to convert an object to a string. More readable. In this case, we are testing the toString function, so e.toString() is probably even better here. http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-string.js#newcode38 test/mjsunit/cyclic-error-to-string.js:38: e.arguments = e; Why .stack and .arguments? They don't seem to affect (and should not be expected to affect) the toString function. It would be better if they actually contained string values that should not occur in the result. I.e. change them to: e.stack = "Does not occur in output"; e.arguments = "Does not occur in output"; e.type = "Does not occur in output"; (adding .type just to be sure). http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-string.js#newcode39 test/mjsunit/cyclic-error-to-string.js:39: assertEquals('', e + ''); Seems a little odd that we can get ": " when neither name nor message is the empty string. I can see that we need to do something, but I'd prefer putting something else than the empty string in place of recurring objects in the toString call graph. E.g. "#<error>". But we match Safari, so let's keep it for now. http://codereview.chromium.org/8341021/diff/1/test/mjsunit/cyclic-error-to-string.js#newcode46 test/mjsunit/cyclic-error-to-string.js:46: assertEquals('', e + ''); Add a test to check the sequence of visible operations of toString, e.g., var seq; function testSeq(name, message) { var e = { get name() { seq.push(1); return (name === undefined) ? name : { toString: function() { seq.push(2); return name; }; }; }, get message() { seq.push(3); return message === undefined ? message : { toString: function() { seq.push(4); return message; } }; } }; return Error.prototype.toString.call(e); } seq = []; assertEquals("Error", testSeq(undefined, undefined)); assertEquals([1,3], seq); seq = []; assertEquals("e1", testSeq("e1", undefined)); assertEquals([1,2,3], seq); seq = []; assertEquals("e2", testSeq(undefined, "e2")); assertEquals([1,3,4], seq); seq = []; assertEquals("e1: e2", testSeq("e1", "e2")); assertEquals([1,2,3,4], seq); (not tested). http://codereview.chromium.org/8341021/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
