Yeah, there was an off by one error in that code (and the instruction should have been emitted before the state update, too). Thanks for catching it and fixing it.
http://codereview.chromium.org/14158/diff/5/205 File src/virtual-frame-ia32.cc (right): http://codereview.chromium.org/14158/diff/5/205#newcode576 Line 576: int new_stack_pointer = i; // Before a final pop. I don't agree that this really makes the code clearer. new_stack_pointer_ is just i in a code block where i doesn't change. I would probably just make the comment immediately above clearer, that we are adjusting the stack pointer to be equal to i. http://codereview.chromium.org/14158/diff/5/205#newcode584 Line 584: Immediate((stack_pointer_ - new_stack_pointer) * kPointerSize)); If we keep the name new_stack_pointer_, we should probably name some part of this long expresstion (stack_pointer_ - new_stack_pointer) * kPointerSize. In any case, the arguments should be aligned. http://codereview.chromium.org/14158/diff/5/205#newcode595 Line 595: SyncRange(stack_pointer_ + 1, i); I actually don't mind syncing zero-sized (or even negative) ranges since it does the right thing. The simplest way to state the precondition for SyncElementAt is that everything between the stack pointer and the element (exclusive on both ends) must be synced, even if that range includes no elements. However, if we change it here, we should probably change it everywhere. http://codereview.chromium.org/14158 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
