Still LGTM. On Wed, Oct 21, 2009 at 11:08 AM, <[email protected]> wrote:
> Uploaded new patch set. > > > > http://codereview.chromium.org/302002/diff/8002/9011 > File src/arm/codegen-arm.h (right): > > http://codereview.chromium.org/302002/diff/8002/9011#newcode202 > Line 202: static InlineRuntimeLUT* FindInlineRuntimeLUT(Handle<String> > name); > On 2009/10/21 07:39:28, Kevin Millikin wrote: > >> You could just make CodeGenSelector a friend of CodeGenerator and not >> > have to > >> expose this in the public interface. >> > > It's a little weird for a public member function to export a pointer >> > to a > >> private one. >> > > Done. > > http://codereview.chromium.org/302002/diff/8002/9006 > File src/arm/fast-codegen-arm.cc (right): > > http://codereview.chromium.org/302002/diff/8002/9006#newcode89 > Line 89: Comment cmnt(masm_, "[ Declarations"); > On 2009/10/21 07:39:28, Kevin Millikin wrote: > >> We have a mix in our codebase of opening brace on a line by itself to >> > start a > >> block and first line of code on the same line as opening brace, so >> > either is > >> acceptable. >> > > However, we probably shouldn't mix them in the same function and code >> > should be > >> formatted to match the code around it, so change this one or the >> > others. (I > >> prefer the other style that saves on vertical space. Economizing on >> > vertical > >> space where appropriate is encouraged by the coding guide.) >> > > Done. > > I also changed the order in which we generate the stack check, call > VisitDeclarations and call VisitStatements to be consistent across all > platforms. > > http://codereview.chromium.org/302002/diff/8002/9004 > File src/fast-codegen.cc (right): > > http://codereview.chromium.org/302002/diff/8002/9004#newcode126 > Line 126: static Handle<Code> ComputeLazyCompile(int argc) { > On 2009/10/21 07:39:28, Kevin Millikin wrote: > >> Can you get rid of this one? >> > > Done. > > http://codereview.chromium.org/302002/diff/8002/9004#newcode138 > Line 138: Handle<Code> code = ComputeLazyCompile(fun->num_parameters()); > On 2009/10/21 07:39:28, Kevin Millikin wrote: > >> Can you call CodeGenerator::ComputeLazyCompile here? >> > > Done. > > > http://codereview.chromium.org/302002 > --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
