Thanks a lot for comments.
http://codereview.chromium.org/6451004/diff/1/src/messages.cc File src/messages.cc (right): http://codereview.chromium.org/6451004/diff/1/src/messages.cc#newcode78 src/messages.cc:78: Factory::NewJSArrayWithElements(arguments_elements); On 2011/02/08 14:17:56, Lasse Reichstein wrote:
Is this fixing anything, or is it just a rewrite?
It can be omitted, I wrote about that in follow up message, sorry, should have posted with request for review. http://codereview.chromium.org/6451004/diff/1/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/6451004/diff/1/src/objects.cc#newcode6967 src/objects.cc:6967: if (result != Heap::the_hole_value()) return result; On 2011/02/08 14:17:56, Lasse Reichstein wrote:
Check if the result is a failure instead. You are comparing pointers of different type here (which is slightly
icky, even
if it's true).
Let me double check. I cannot use plain IsFailure as I need to be able to distinguish between three outcomes: no setter in proto chain, setting threw, setting went fine. I used this hole to signify no setter case. I can first check it's not a failure and when compare Object to the hole. I can too pass a pointer to a flag. Which one do you prefer? http://codereview.chromium.org/6451004/diff/1/src/objects.cc#newcode7102 src/objects.cc:7102: if (result != Heap::the_hole_value()) return result; On 2011/02/08 14:17:56, Lasse Reichstein wrote:
Same here. Check result->IsFailure()
See above. http://codereview.chromium.org/6451004/diff/1/test/mjsunit/getter-in-prototype.js File test/mjsunit/getter-in-prototype.js (right): http://codereview.chromium.org/6451004/diff/1/test/mjsunit/getter-in-prototype.js#newcode58 test/mjsunit/getter-in-prototype.js:58: this[0] = 42; On 2011/02/08 14:17:56, Lasse Reichstein wrote:
I would think "this" here refers to the global object, not to "o". Can
you say
why this works?
Very good catch. That works as we set p to be a proto of global object above (ln. 45). I think I'll just drop this case and reorganize tests slightly. Thanks a lot. http://codereview.chromium.org/6451004/diff/1/test/mjsunit/regress/regress-1107.js File test/mjsunit/regress/regress-1107.js (right): http://codereview.chromium.org/6451004/diff/1/test/mjsunit/regress/regress-1107.js#newcode32 test/mjsunit/regress/regress-1107.js:32: assertThrows("x.x"); On 2011/02/08 14:17:56, Lasse Reichstein wrote:
Should the property name be "x", not 0?
Copy paste from original report. Actually, it uses x just to throw an exception. I can simplify it to "x" instead of "x.x". Will do http://codereview.chromium.org/6451004/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
