Much nicer now.
http://codereview.chromium.org/50012/diff/7/12 File src/virtual-frame-arm.h (right): http://codereview.chromium.org/50012/diff/7/12#newcode450 Line 450: // Presently does nothing on ARM. On 2009/03/20 14:44:56, Kevin Millikin wrote: > This comment should just be identical to the IA32 one (mutatis mutandis). "Does > nothing on ARM" has to be maintained (and isn't correct anyway---calling it does > something by terminating the process in release and debug builds). Done. http://codereview.chromium.org/50012/diff/7/8 File src/virtual-frame-ia32.cc (right): http://codereview.chromium.org/50012/diff/7/8#newcode501 Line 501: // Before changing an element which is copied, adjust so that the On 2009/03/20 14:44:56, Kevin Millikin wrote: > Change this comment to correspond to the changes to the code. Done. http://codereview.chromium.org/50012/diff/7/8#newcode506 Line 506: FrameElement VirtualFrame::InvalidateFrameSlot(int index) { On 2009/03/20 14:44:56, Kevin Millikin wrote: > To match the names of everything else, this should be the slightly longer > "InvalidateFrameSlotAt". Done. http://codereview.chromium.org/50012/diff/7/8#newcode510 Line 510: // Set it to invalid, and free the register. On 2009/03/20 14:44:56, Kevin Millikin wrote: > This comment is confusing. It mentions freeing a register (the original may not > even be a register), but I don't see any code to do that. Done. http://codereview.chromium.org/50012/diff/7/8#newcode512 Line 512: elements_[index].clear_copied(); On 2009/03/20 14:44:56, Kevin Millikin wrote: > Invalid elements (in fact, all new elements) are already created with their copy > flags cleared, so this line isn't needed. Done. http://codereview.chromium.org/50012/diff/7/8#newcode523 Line 523: Unuse(original.reg()); On 2009/03/20 14:44:56, Kevin Millikin wrote: > I think it's more clear to have the code to set elements_[index] to invalid > here, close to the unuse of the register (even if it means duplicating that line > for the other case). It's too hard to track related state changes that are > separated too much in the code. Done. http://codereview.chromium.org/50012/diff/7/8#newcode541 Line 541: FrameElement::SyncField::decode(elements_[i].type())); On 2009/03/20 14:44:56, Kevin Millikin wrote: > Even though VirtualFrame is a friend of FrameElement, it shouldn't depend on the > representation of FrameElements (that it's a bit field). Go through the > accessors (or add a new one if you need). Done. http://codereview.chromium.org/50012/diff/7/8#newcode544 Line 544: FrameElement copy = CopyElementAt(i); On 2009/03/20 14:44:56, Kevin Millikin wrote: > This has a slight drawback of marking the new backing store as copied even if > it's not. Done. http://codereview.chromium.org/50012/diff/7/8#newcode553 Line 553: return copy; On 2009/03/20 14:44:56, Kevin Millikin wrote: > It seems like there's a slicker way to do this. > * Hold off marking the new backing store as copied until the loop looking for > other copies finds one (sometimes we want a copy as the return value, but other > times we will just ignore it anyway). > * For all the copies, can't we just set their data_.index_ member to be the new > backing store (that would automatically preserve the sync flag)? > * Instead of returning a copy of the new backing store or invalid if the > original wasn't copied, can't we just return the index of the new backing store > or kIllegalIndex, and then decide whether to copy it in the caller (where we > know if we want a copy)? Done. http://codereview.chromium.org/50012/diff/7/8#newcode620 Line 620: elements_[index] = FrameElement::InvalidElement(); On 2009/03/20 14:44:56, Kevin Millikin wrote: > If you made InvalidateFrameSlot work on copies and constants, you wouldn't need > this line? Done. http://codereview.chromium.org/50012/diff/7/8#newcode632 Line 632: // If the stored-to slot may be copied, adjust to preserve the On 2009/03/20 14:44:56, Kevin Millikin wrote: > Adjust this comment to reflect the new code. Done. http://codereview.chromium.org/50012/diff/7/8#newcode634 Line 634: if (original.is_register() || original.is_memory()) { On 2009/03/20 14:44:56, Kevin Millikin wrote: > It's not obvious why InvalidateFrameSlot won't do the right thing for > non-register, non-memory frame elements. Done. http://codereview.chromium.org/50012/diff/7/10 File src/virtual-frame-ia32.h (right): http://codereview.chromium.org/50012/diff/7/10#newcode436 Line 436: // Helper function to implement the copy-on-write semantics of an On 2009/03/20 14:44:56, Kevin Millikin wrote: > This comment needs to be updated: the main point is that it invalidates a frame > element (not necessarily just before writing to it anymore), unusing registers > and preserving the copy invariants as needed. Done. http://codereview.chromium.org/50012/diff/7/9 File src/virtual-frame.cc (right): http://codereview.chromium.org/50012/diff/7/9#newcode380 Line 380: // semantics of copied elements. On 2009/03/20 14:44:56, Kevin Millikin wrote: > Change this comment, it's no longer correct. Done. http://codereview.chromium.org/50012/diff/7/9#newcode381 Line 381: if (original.is_register() || original.is_memory()) { On 2009/03/20 14:44:56, Kevin Millikin wrote: > It's not obvious why InvalidateFrameSlot won't do the right thing for > non-register, non-memory frame elements, or is it just that it's unnecessary > (the comment should explain). Done. http://codereview.chromium.org/50012 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
