http://codereview.chromium.org/16579/diff/1/3 File src/codegen-ia32.cc (right):
http://codereview.chromium.org/16579/diff/1/3#newcode306 Line 306: if (function_return_.is_linked()) { On 2009/01/08 06:55:13, Kasper Lund wrote: > Should (or could) this really be an else if? (It could and) maybe it should. I've changed it. http://codereview.chromium.org/16579/diff/1/3#newcode1682 Line 1682: frame_->Push(eax); // Materialize result on the stack. On 2009/01/08 06:55:13, Kasper Lund wrote: > Is this really okay when the frame is invalid? Can you run the regression test > with --trace? It's OK here because we actually have a valid frame. I've refactored to make it clearer and tested --trace. http://codereview.chromium.org/16579/diff/1/3#newcode1687 Line 1687: Label check_exit_codesize; On 2009/01/08 06:55:13, Kasper Lund wrote: > The more I look at this size checking construct, there more I feel it needs an > abstraction. How about having a stack-allocated helper that verifies the > generated code size? It seems like it's only used in this one place, but > whenever I read this code I wonder who jumps to the label that we explicitly > bind. I think it's OK. The label's lifetime ends pretty quickly, so it's easy to see that nobody jumps to it. With JumpTargets labeling basic blocks, the vanilla Labels are only used for local things like this, so that should also help the reader. http://codereview.chromium.org/16579 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
