I'll come up with a regression test too. On Thu, Jan 8, 2009 at 10:32 AM, <[email protected]> wrote:
> > 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 -~----------~----~----~----~------~----~------~--~---
