I like the cleanup/simplification of ia32 and x64. They look good. I think the ARM should be cleaned up in the same way.
There are some coding style comments. One other comment is that it's probably a good idea to try to make the comments the same (where reasonable) on all three platforms. We don't want someone to have to read the ia32 version to get the good comments. We also don't want people to wonder if differences are important or superficial. http://codereview.chromium.org/316009/diff/7002/7008 File src/arm/fast-codegen-arm.cc (right): http://codereview.chromium.org/316009/diff/7002/7008#newcode228 Line 228: // If so, jump to the deferred code. The comment about deferred code is out of date. http://codereview.chromium.org/316009/diff/7002/7008#newcode242 Line 242: __ push(r0); There's an opportunity to combine the three pushes into a stm by choosing a higher register index than r1 for the literals array. I think you can just swap the use of r1 and r2 in the code before and including the undefined check. http://codereview.chromium.org/316009/diff/7002/7008#newcode255 Line 255: __ push(r0); I think you can skip this push and use code more like the ia32, keeping the object in r0 for each spin of the code generation loop. http://codereview.chromium.org/316009/diff/7002/7008#newcode268 Line 268: case ObjectLiteral::Property::PROTOTYPE: { No need for block. http://codereview.chromium.org/316009/diff/7002/7008#newcode275 Line 275: Visit(value); Assert this is on stack (location is_temporary for now). http://codereview.chromium.org/316009/diff/7002/7008#newcode277 Line 277: __ ldr(r0, MemOperand(sp)); The value restored to r0 from the stack is the same as the one already in r0 returned by the runtime. http://codereview.chromium.org/316009/diff/7002/7008#newcode281 Line 281: case ObjectLiteral::Property::GETTER: { No need for block. http://codereview.chromium.org/316009/diff/7002/7008#newcode292 Line 292: Visit(value); Assert this is on stack. http://codereview.chromium.org/316009/diff/7002/7008#newcode294 Line 294: __ ldr(r0, MemOperand(sp)); Same comment. I don't think there is a need to save the object on the stack or restore it. http://codereview.chromium.org/316009/diff/7002/7008#newcode299 Line 299: if (expr->location().is_nowhere()) { Here you would do nothing for nowhere and push r0 for temporary. http://codereview.chromium.org/316009/diff/7002/7004 File src/ia32/fast-codegen-ia32.cc (right): http://codereview.chromium.org/316009/diff/7002/7004#newcode245 Line 245: bool result_saved = false; I think the code below reads a little better if you call this 'object_in_eax' or 'object_in_reg' or something instead. (Ie, reverse the sense of the boolean.) http://codereview.chromium.org/316009/diff/7002/7004#newcode256 Line 256: if (!result_saved) { if (object_in_eax) { __ push(eax); object_in_eax = false; } http://codereview.chromium.org/316009/diff/7002/7004#newcode261 Line 261: case ObjectLiteral::Property::MATERIALIZED_LITERAL: // fall through We indent case labels two spaces (and their bodies a further two spaces). http://codereview.chromium.org/316009/diff/7002/7004#newcode263 Line 263: case ObjectLiteral::Property::COMPUTED: { No need for a block around this case. http://codereview.chromium.org/316009/diff/7002/7004#newcode276 Line 276: case ObjectLiteral::Property::PROTOTYPE: { No need for a block around this one. http://codereview.chromium.org/316009/diff/7002/7004#newcode285 Line 285: result_saved = false; object_in_eax = true; http://codereview.chromium.org/316009/diff/7002/7004#newcode289 Line 289: case ObjectLiteral::Property::GETTER: { No need for block. http://codereview.chromium.org/316009 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
