Ivan, Thanks for the comments. I definitely agree about GenerateStoreCode---right now I only passed in what it needed. Perhaps it should really take a CodeGenState, and CodeGenState should encapsulate exactly everything a static CodeGenerator function or non-member function needs. Right now, there is stuff that we would need that is not in the state but in the code generator itself.
I will incorporate your comments tomorrow morning. regards, Kevin On Wed, Sep 10, 2008 at 8:01 PM, <[EMAIL PROTECTED]> wrote: > > Kevin, > > For now it is OK to have the GenerateStoreCode methods take a scope > parameter. I can envision that at some point these methods might need to > take a CodeGenerator parameter to tailor the code based on particular > state of the compilation such as variable or frame layout. Such > information is not conveyed as part of the Scope. > > -Ivan > > > http://codereview.chromium.org/1889/diff/1/2 > File src/ast.h (right): > > http://codereview.chromium.org/1889/diff/1/2#newcode164 > Line 164: bool is_const_init) { > Can you please use an enum for the is_const_init to improve the > readability at the caller sites? > > http://codereview.chromium.org/1889/diff/1/3 > File src/codegen-arm.cc (right): > > http://codereview.chromium.org/1889/diff/1/3#newcode2033 > Line 2033: ASSERT(!ref->is_illegal()); > Nit: Don't you want to assert before accessing any fields of ref? > > http://codereview.chromium.org/1889/diff/1/3#newcode3295 > Line 3295: // CodeGenState::LOAD_TYPEOF_EXPR. > Please assert that the CodeGenState is LOAD_TYPEOF_EXPR. That way you > ensure that you are not entering here with UNDEFINED or a newly added > state. > > http://codereview.chromium.org/1889/diff/1/4 > File src/codegen-ia32.cc (right): > > http://codereview.chromium.org/1889/diff/1/4#newcode1141 > Line 1141: ASSERT(!ref()->is_illegal()); > Same nit as on ARM: Don't you want to assert before accessing ref? > > http://codereview.chromium.org/1889/diff/1/4#newcode3496 > Line 3496: // CodeGenState::LOAD_TYPEOF_EXPR. > Same comment as on ARM: Please assert that the CodeGenState is > LOAD_TYPEOF_EXPR. That way you ensure that you are not entering here > with UNDEFINED or a newly added state. > > http://codereview.chromium.org/1889 > > > > -- Google Denmark ApS CVR nr. 28 86 69 84 c/o Philip & Partners, 7 Vognmagergade, P.O. Box 2227, DK-1018 Copenhagen K, Denmark --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
