LGTM. Couple formatting issues you should look at.
http://codereview.chromium.org/50012/diff/1007/1008 File src/virtual-frame-ia32.cc (right): http://codereview.chromium.org/50012/diff/1007/1008#newcode537 Line 537: elements_[new_backing_index] = FrameElement::RegisterElement(backing_reg, I still don't like this screwy indentation. What's wrong with: if (elements_[new_backing_index].is_synced()) { elements_[new_backing_index] = FrameElement::RegisterElement(backing_reg, FrameElement::SYNCED); } else { ... } If you stick with the ternary operator, please follow the style guide (break after the opening paren, align each new line containing arguments with a four space indent, and though not required, don't align the ? and : lines so they look like arguments (ie, indent them further)). http://codereview.chromium.org/50012/diff/1007/1008#newcode543 Line 543: for (int j = new_backing_index+1; j < elements_.length(); j++) { I'd rather see "i" for the index. Binary operators (+) should be surrounded with spaces. http://codereview.chromium.org/50012/diff/1007/1008#newcode566 Line 566: // that register element on the top of the stack. Be careful in comments about the distinction between "stack" (which exists at runtime) and "(virtual) frame" which exists at compile time. Here we're actually not pushing on the stack, so I'd say something like "allocate to a register and push on the frame". http://codereview.chromium.org/50012 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
