Added new patch set. PTAQL.
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;
On 2011/10/26 07:40:24, Lasse Reichstein wrote:
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").
I agree, the spec says nothing about any "type" or "arguments" property.
I think the only correct solution would be to hide those two properties
completely from the JavaScript world. But that is outside the scope of
this CL.
http://codereview.chromium.org/8341021/diff/1/src/messages.js#newcode1158
src/messages.js:1158: var name = error.name
On 2011/10/26 07:40:24, Lasse Reichstein wrote:
According to the spec, we should do ToString on name before reading
message.
Done.
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.
On 2011/10/26 07:40:24, Lasse Reichstein wrote:
We should check whether this is really (still) the case.
Done (no longer needed, removed).
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#newcode30
test/mjsunit/cyclic-error-to-string.js:30:
On 2011/10/26 08:50:44, Lasse Reichstein wrote:
So we have a test of Error.toString that tests that it does what it
should in
the non-cyclic case?
I.e., that it does the correct conversion using ToString if not
undefined for
name and message (and that, e.g., null isn't treated the same as
undefined).
Done. I have renamed this test case to error-tostring.js, because it now
does way more than just testing the cyclic case.
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 +
'');
On 2011/10/26 07:40:24, Lasse Reichstein wrote:
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.
Done.
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;
On 2011/10/26 07:40:24, Lasse Reichstein wrote:
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).
Done.
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 + '');
On 2011/10/26 07:40:24, Lasse Reichstein wrote:
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.
I agree, having a more sensible string representation for the cyclic
case would be good. But since it is not specified how to treat cycles, I
think staying in sync with Firefox (and probably Safari once they fix
the ES5 issue here) is a good thing.
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 + '');
On 2011/10/26 07:40:24, Lasse Reichstein wrote:
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).
Done (added with slight modifications).
http://codereview.chromium.org/8341021/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
