I've added a unit test to show we haven't messed up top-level assignments in addition to fixing the issues below.
http://codereview.chromium.org/27128/diff/1/6 File src/codegen-ia32.cc (right): http://codereview.chromium.org/27128/diff/1/6#newcode2764 Line 2764: // Changing to slow case in the beginning of an initialization block On 2009/02/25 13:11:07, Mads Ager wrote: > Change to slow case ... to avoid ... > or > Changing to slow case ... avoids ... > ? Fixed http://codereview.chromium.org/27128/diff/1/5 File src/parser.cc (right): http://codereview.chromium.org/27128/diff/1/5#newcode74 Line 74: static const int kMinInitializationBlock = 3; On 2009/02/25 13:11:07, Mads Ager wrote: > Is this limit based on benchmarking? We should make sure to only perform the > two runtime calls if that overhead is less than the instance descriptor copying > overhead. It's based on benchmarking. I've added a comment to that effect. http://codereview.chromium.org/27128/diff/1/5#newcode1242 Line 1242: if (!InBlock() && (assignment != NULL)) StartBlock(assignment); On 2009/02/25 13:11:07, Mads Ager wrote: > In BlockContinues, you check if the operation is Token::ASSIGN. You should do > that here (or in StartBlock) as well? > Please add a test case that hits this. Fixed. http://codereview.chromium.org/27128/diff/1/5#newcode1269 Line 1269: if (assignment->op() != Token::ASSIGN) return false; On 2009/02/25 12:54:45, iposva wrote: > You check here that only ASSIGN statements are added when the block continues, > but you do not check for the initial node to be an ASSIGN statement anywhere I > can tell. Fixed. http://codereview.chromium.org/27128/diff/1/5#newcode1304 Line 1304: bool InitializationBlockFinder::SameObject(Expression* e1, Expression* e2) { On 2009/02/25 12:54:45, iposva wrote: > What made you put this method outside the class declaration? Since this is a > private class I would just put all of the code in the declaration, or at least > pull all non-trivial (aka 1-liners) out to be consistent. I've put all the code in the declaration for consistency. http://codereview.chromium.org/27128/diff/1/5#newcode1307 Line 1307: if (v1 != NULL && v2 != NULL) { On 2009/02/25 12:54:45, iposva wrote: > In general I find code more readable when () are added around boolean > expressions such as > if ((v1 != NULL) && (v2 != NULL)) Fixed where found. http://codereview.chromium.org/27128/diff/1/5#newcode1345 Line 1345: // degradation in any other scopes. On 2009/02/25 13:11:07, Mads Ager wrote: > You should write here that the reason for only doing this in top level code is > that map transitions are not used in this case leading to different maps for all > objects going through this series of assignments. Fixed. http://codereview.chromium.org/27128 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
