It basically looks right but I have some picky comments.
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(); I'm not sure why we spill here. Perhaps a comment explaining why it's necessary? http://codereview.chromium.org/18616/diff/1/4#newcode196 Line 196: context.Unuse(); It's weird that context's lifetime is different depending on whether kDebug is set or not, but I guess it's OK. http://codereview.chromium.org/18616/diff/1/4#newcode865 Line 865: Result arg(generator()); "arg" isn't a good name since it's multiplexed for things that aren't the argument. Use a more generic name ("result") if you want to reuse the same result, or just introduce another one. http://codereview.chromium.org/18616/diff/1/4#newcode868 Line 868: generator()->frame()->Spill(arg.reg()); // Should not be needed. 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? http://codereview.chromium.org/18616/diff/1/4#newcode870 Line 870: Immediate immediate(Smi::FromInt(value_)); Since immediate is only used once, why not inline it? http://codereview.chromium.org/18616/diff/1/4#newcode1011 Line 1011: frame_->Push(&operand); 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. 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. 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". http://codereview.chromium.org/18616 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
