LGTM with one comment. (Adam, please feel free to land should I not be
around
later.)
https://codereview.chromium.org/15504002/diff/33001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode11031
src/objects.cc:11031: uint32_t add_count = new_length > old_length ?
new_length - old_length : 0;
On 2013/05/28 11:38:53, rafaelw wrote:
new_length and old_length are both uint so, without casting,
(new_length -
old_length) is never going to be less than 0. I could cast to ints,
but I'm not
sure that buys much since Max is just a macro.
Leaving for now. Please clarify if you want something else.
On 2013/05/27 14:48:41, rossberg wrote:
> Max(new_length - old_length, 0)
OK, I see.
https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode11036
src/objects.cc:11036: while (i--) {
On 2013/05/28 11:38:53, rafaelw wrote:
I'm curious why you prefer the for loop, but I've changed it for you.
On 2013/05/27 14:48:41, rossberg wrote:
> Can we make this a for loop?
I'm curious why you're curious. ;) It is just an indexed loop from i-1
to 0, isn't it? Why would it be preferable not to express this as a for
loop?
https://codereview.chromium.org/15504002/diff/33001/src/v8natives.js
File src/v8natives.js (right):
https://codereview.chromium.org/15504002/diff/33001/src/v8natives.js#newcode929
src/v8natives.js:929: new_length < old_length ? new_length : old_length,
On 2013/05/28 11:38:53, rafaelw wrote:
Math. doesn't seem to be available from here. If there's a way to
access the
impl, let me know how.
You're right, Math is defined later in the bootstrapping process.
https://codereview.chromium.org/15504002/diff/33001/test/mjsunit/harmony/object-observe.js
File test/mjsunit/harmony/object-observe.js (right):
https://codereview.chromium.org/15504002/diff/33001/test/mjsunit/harmony/object-observe.js#newcode1005
test/mjsunit/harmony/object-observe.js:1005: { object: arr3, name:
'length', type: 'reconfigured' }
On 2013/05/28 11:38:53, rafaelw wrote:
It isn't correct, but Adam and I discussed and decided that
a) It's fine to break with spec compliance here because it's an
extreme edge
case and even if hit, two records vs one is almost certain not to
confuse any
observer.
b) Making it spec compliant in this minor way isn't worth layering
hacks on top
of more hacks here. Once the todo mentioned by mstarzinger is fixed,
the right
change record sequence will return.
On 2013/05/27 14:48:41, rossberg wrote:
> Not sure I understand, why is it correct that this a separate change
now?
OK, can you add a TODO then?
https://codereview.chromium.org/15504002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
