Feedback addressed
http://codereview.chromium.org/9663002/diff/2001/src/builtins.cc File src/builtins.cc (left): http://codereview.chromium.org/9663002/diff/2001/src/builtins.cc#oldcode511 src/builtins.cc:511: AssertNoAllocation no_gc; The AssertNoAllocation doesn't help here, and actually it's misleading (the code here is wrong as it was). There can't be an allocation between the time that new_elms is allocated and the copy happens. But the AssertNoAllocation object doesn't guarantee that, it only guarantees that there is no allocation since the no allocation object is created. I talked about this with Starzinger, and we agreed that it's misleading to suggest that there is some sort of protection through stack-allocated protection objects when there isn't. In other new code, the WriteBarrierMode is explicit. Until there is a replacement for AssertNoAllocation that actually is bound tighter to the allocation that it's trying to protect (that's a bigger chunk of work), the explicit mode at least forces the caller to think about what is safe and what is not. On 2012/03/12 15:33:28, Jakob wrote:
Did you delete this line on purpose? AFAICS we don't allocate anything
during
the call, so we might want to keep the assertion.
http://codereview.chromium.org/9663002/diff/2001/src/builtins.cc#oldcode976 src/builtins.cc:976: AssertNoAllocation no_gc; See above. On 2012/03/12 15:33:28, Jakob wrote:
Same question here.
http://codereview.chromium.org/9663002/diff/2001/src/elements.cc File src/elements.cc (right): http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode151 src/elements.cc:151: #if DEBUG On 2012/03/12 15:33:28, Jakob wrote:
s/#if/#ifdef/
Again below.
Done. http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode157 src/elements.cc:157: ASSERT(to->get(i) == hole); On 2012/03/12 15:33:28, Jakob wrote:
ASSERT(to->is_the_hole(i))?
Done. http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode162 src/elements.cc:162: ASSERT(((copy_size + static_cast<int>(to_start)) <= to->length() && On 2012/03/12 15:33:28, Jakob wrote:
nit: you can remove at least one pair of parentheses.
Done. http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode202 src/elements.cc:202: ASSERT(to->get(i) == hole); On 2012/03/12 15:33:28, Jakob wrote:
ASSERT(to->is_the_hole(i))?
Done. http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode210 src/elements.cc:210: ASSERT(copy_size > 0 || copy_size == ElementsAccessor::kCopyToEnd || On 2012/03/12 15:33:28, Jakob wrote:
I don't see how copy_size can ever be < 0 at this point. It can be ==
0,
however.
Done. http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode212 src/elements.cc:212: ASSERT(copy_size + static_cast<int>(to_start) <= to->length()); On 2012/03/12 15:33:28, Jakob wrote:
Duplicate assertion (see line 207).
Done. http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode221 src/elements.cc:221: to->set(i + to_start, hole, SKIP_WRITE_BARRIER); On 2012/03/12 15:33:28, Jakob wrote:
to->set_the_hole(i + to_start)?
Done. http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode255 src/elements.cc:255: ASSERT(to->get(i) == hole); On 2012/03/12 15:33:28, Jakob wrote:
ASSERT(to->is_the_hole(i))?
Done. http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode260 src/elements.cc:260: ASSERT(((copy_size + static_cast<int>(to_start)) <= to->length() && On 2012/03/12 15:33:28, Jakob wrote:
nit: you can remove at least one pair of parentheses.
Done. http://codereview.chromium.org/9663002/diff/2001/src/elements.cc#newcode338 src/elements.cc:338: ASSERT(((copy_size + static_cast<int>(to_start)) <= to->length() && On 2012/03/12 15:33:28, Jakob wrote:
nit: you can remove at least one pair of parentheses.
Done. http://codereview.chromium.org/9663002/diff/2001/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/9663002/diff/2001/src/objects.cc#newcode8543 src/objects.cc:8543: if (elements_kind != NON_STRICT_ARGUMENTS_ELEMENTS) { I think it's theoretically possible, it least it may be in the future. The motivation here is that this code should look just like the FixedArray case so they can be merged together. There is no reason not to support this case, but you're right, the later map and elements set need to be removed. On 2012/03/12 15:33:28, Jakob wrote:
I don't think this if/else block makes sense. Can
NON_STRICT_ARGUMENTS_ELEMENTS
really be in DOUBLE mode? Also, there are unconditional calls "set_map(new_map);" and "set_elements(elems);" below, which contradict
what we
do up here.
http://codereview.chromium.org/9663002/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
