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. On 2008/12/17 09:50:12, Kevin Millikin wrote: > 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. Done. http://codereview.chromium.org/14158/diff/5/205#newcode584 Line 584: Immediate((stack_pointer_ - new_stack_pointer) * kPointerSize)); On 2008/12/17 09:50:12, Kevin Millikin wrote: > 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. Done. http://codereview.chromium.org/14158/diff/5/205#newcode595 Line 595: SyncRange(stack_pointer_ + 1, i); On 2008/12/17 09:50:12, Kevin Millikin wrote: > 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. I don't understand why we need to change it other places than here. All that is changing is the check if the range is empty. I know it does the right thing, but I think it takes thought to understand why the limit on the test is different than the limit of the range. http://codereview.chromium.org/14158 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
