Comments addressed or explained.
http://codereview.chromium.org/18616/diff/1/4 File src/codegen-ia32.cc (right): http://codereview.chromium.org/18616/diff/1/4#newcode192 Line 192: frame_->SpillAll(); On 2009/01/21 15:09:59, Kevin Millikin wrote: > I'm not sure why we spill here. Perhaps a comment explaining why it's > necessary? It is needed for the breakpoint below. If the spill is only on the breakpoint branch, then the Bind below it will undo the spill, which is probably not worth it. I added a comment. http://codereview.chromium.org/18616/diff/1/4#newcode868 Line 868: generator()->frame()->Spill(arg.reg()); // Should not be needed. On 2009/01/21 15:09:59, Kevin Millikin wrote: > The comment is strange. Do you mean that it is not actually needed (why is it > there?)? Do you mean that it is actually needed but it is possible to eliminate > that need (if so, make it a TODO or just do it). Do you mean that it is a > precondition (then ASSERT it and structure the caller so it's true)? Do you > mean that you don't know if it's needed or not? It is defensive programming. It is currently not needed, but changes in the caller could make it needed. The comment is removed. Done. http://codereview.chromium.org/18616/diff/1/4#newcode870 Line 870: Immediate immediate(Smi::FromInt(value_)); On 2009/01/21 15:09:59, Kevin Millikin wrote: > Since immediate is only used once, why not inline it? Done. http://codereview.chromium.org/18616/diff/1/4#newcode1011 Line 1011: frame_->Push(&operand); On 2009/01/21 15:09:59, Kevin Millikin wrote: > This code works because virtual frame operations all work if the frame happens > to be spilled (they just don't preserve a spilled frame). However, it would be > more explicit to push the spilled scope down into the other arms of the switch, > especially since we are using spilled scopes primarily to mark code which has > not been updated. The next changelist does this. http://codereview.chromium.org/18616/diff/1/3 File src/virtual-frame-ia32.h (right): http://codereview.chromium.org/18616/diff/1/3#newcode296 Line 296: // Push the address of the frame slot containing the receiver on the frame. On 2009/01/21 15:09:59, Kevin Millikin wrote: > Frame slots don't have addresses (well, they do but we don't rely on them). I > think this should be "stack slot" or even simpler "receiver slot". Done. http://codereview.chromium.org/18616 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
