LGTM.
http://codereview.chromium.org/40295/diff/1/3 File src/ast.h (right): http://codereview.chromium.org/40295/diff/1/3#newcode639 Line 639: explicit MaterializedLiteral(int literal_index, bool is_simple) Doesn't need to be explicit anymore, but I guess it doesn't hurt. http://codereview.chromium.org/40295/diff/1/3#newcode667 Line 667: CONSTANT, // Property with constant value (compile time). Align comments? http://codereview.chromium.org/40295/diff/1/10 File src/codegen-ia32.cc (right): http://codereview.chromium.org/40295/diff/1/10#newcode3454 Line 3454: case ObjectLiteral::Property::COMPUTED: { Maybe mention in a comment that there's a 'fall through' here. http://codereview.chromium.org/40295/diff/1/10#newcode3512 Line 3512: // by calling Runtime_CreateObjectLiteral. Runtime_CreateArrayLiteralBoilerplate? http://codereview.chromium.org/40295/diff/1/10#newcode3586 Line 3586: frame_->CallRuntime(Runtime::kCloneObjectLiteralBoilerplate, 1); Maybe the name should be generalized a bit here to indicate that is is also used to clone array literals? http://codereview.chromium.org/40295/diff/1/9 File src/parser.cc (right): http://codereview.chromium.org/40295/diff/1/9#newcode3101 Line 3101: if (object_literal != NULL && object_literal->is_simple()) { Two spaces after NULL. http://codereview.chromium.org/40295/diff/1/9#newcode3106 Line 3106: if (array_literal != NULL && array_literal->is_simple()) { Can an expression convert to an array literal and an object literal? The code seems to indicate that, and if it's not the case, I would probably make that clear by wrapping the array literal case in an else clause or something. Remove extra space after NULL here too. http://codereview.chromium.org/40295/diff/1/9#newcode3110 Line 3110: return result; I guess you're sure that it's either an object literal or an array literal, right? Maybe that could be more clear from the code. http://codereview.chromium.org/40295/diff/1/7 File src/parser.h (right): http://codereview.chromium.org/40295/diff/1/7#newcode171 Line 171: class CompileTimeValue { Inherit from AllStatic? http://codereview.chromium.org/40295/diff/1/4 File src/runtime.cc (right): http://codereview.chromium.org/40295/diff/1/4#newcode276 Line 276: Handle<FixedArray> literals = args.at<FixedArray>(0); Is there a reason why you can't use the CONVERT_ARG_CHECKED macros here (and maybe use CONVERT_CHECKED for the index)? I guess that it's somewhat closer to the CreateObjectLiteralBoilerplate this way... http://codereview.chromium.org/40295 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
