Made all recommended changes, committed. On Mon, Mar 23, 2009 at 2:14 PM, <[email protected]> wrote:
> 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 > -- We can IMAGINE what is not --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
