PTAL. Also, please land if lgtm.
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;
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)
https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode11032
src/objects.cc:11032: uint32_t delete_count = new_length < old_length ?
old_length - new_length : 0;
Same here.
On 2013/05/27 14:48:41, rossberg wrote:
Max(old_length - new_length, 0)
https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode11034
src/objects.cc:11034: if (delete_count) {
On 2013/05/27 14:48:41, rossberg wrote:
Nit: delete_count > 0
Done.
https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode11036
src/objects.cc:11036: while (i--) {
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?
https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode12069
src/objects.cc:12069: Handle<Object> new_length_handle;
On 2013/05/27 14:48:41, rossberg wrote:
Move this and new_length into conditional below, to avoid additional
register
pressure on ia32.
Done.
https://codereview.chromium.org/15504002/diff/33001/src/objects.cc#newcode12095
src/objects.cc:12095: bool array_extended = false;
On 2013/05/27 14:48:41, rossberg wrote:
Can we initialize this properly instead of mutating later, i.e.:
bool array_extended = self->IsJSArray() && ...;
But actually, I'd simplify control flow by making it one 'if/else' and
duplicate
the single EnqueueChangeRecord call.
Done.
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,
Math. doesn't seem to be available from here. If there's a way to access
the impl, let me know how.
On 2013/05/27 14:48:41, rossberg wrote:
Perhaps use Math.min
https://codereview.chromium.org/15504002/diff/33001/src/v8natives.js#newcode932
src/v8natives.js:932: new_length > old_length ? new_length - old_length
: 0);
Ditto.
On 2013/05/27 14:48:41, rossberg wrote:
Perhaps use Math.max
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' }
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?
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.
