All comments so far, addressed. Passes all tests.
http://codereview.chromium.org/17380/diff/5/6 File src/codegen-ia32.cc (right): http://codereview.chromium.org/17380/diff/5/6#newcode753 Line 753: // The arguments to the binary operation are passed in on the frame (stack). On 2009/01/15 10:28:57, Kevin Millikin wrote: > Unclear. They may or may not be on the stack at run time, and there is no stack > at compile time. "The arguments are on top of the code generator's frame." Done. http://codereview.chromium.org/17380/diff/5/6#newcode763 Line 763: Result eax_reg = generator()->allocator()->Allocate(eax); On 2009/01/15 10:28:57, Kevin Millikin wrote: > Allocate the result in VirtualFrame::CallStub and return it, rather than at > every call site. That gives a single place to change it rather than a bunch. > > Just call it "result" or something. Even though it might be eax, we don't care > here. Done. http://codereview.chromium.org/17380/diff/5/6#newcode813 Line 813: // TODO(): When this code is updated to use the virtual frame, it On 2009/01/15 10:28:57, Kevin Millikin wrote: > Remove this comment. Done. http://codereview.chromium.org/17380/diff/5/6#newcode4784 Line 4784: // This function's implementation is a copy of On 2009/01/15 10:28:57, Kevin Millikin wrote: > This comment is no longer true as of this change. Please update. Done. http://codereview.chromium.org/17380/diff/5/6#newcode4805 Line 4805: Result right = frame->Pop(); // Get the right input to the operation. On 2009/01/15 10:28:57, Kevin Millikin wrote: > The comment doesn't seem to say any more (in fact, it says less) than the code > itself. Done. http://codereview.chromium.org/17380/diff/5/6#newcode4809 Line 4809: // Temp is used to compute the answer, leaving left and right unchanged. On 2009/01/15 10:28:57, Kevin Millikin wrote: > Then it's not just "temp". Call it result or answer, maybe? Done. http://codereview.chromium.org/17380/diff/5/6#newcode4842 Line 4842: // Remove tag from one of the operands (but keep sign). On 2009/01/15 10:28:57, Kevin Millikin wrote: > Not just one of the arguments, but the left one. Done. http://codereview.chromium.org/17380/diff/5/6#newcode4844 Line 4844: // We no longer require that the destination be eax. On 2009/01/15 10:28:57, Kevin Millikin wrote: > Document the code as currently written, not what we used to do. This comment > makes little sense to the reader. Done. http://codereview.chromium.org/17380/diff/5/6#newcode4853 Line 4853: Label non_zero_product_mul; On 2009/01/15 10:28:57, Kevin Millikin wrote: > Comment why it's safe to have a raw Label here (no frame effect on either > branch). Done. http://codereview.chromium.org/17380 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
