Doesn't this change lead to duplicate object literal boilerplates for nested object literals?
http://codereview.chromium.org/39184/diff/1/3 File src/ast.h (right): http://codereview.chromium.org/39184/diff/1/3#newcode698 Line 698: bool is_constant_; Should we call this something like 'simple' or 'trivial' instead and explain what we mean by that (only contains constants and other 'simple' object literals)? http://codereview.chromium.org/39184/diff/1/6 File src/parser.cc (right): http://codereview.chromium.org/39184/diff/1/6#newcode3086 Line 3086: Handle<Object> Parser::GetBoilerplateValue(ObjectLiteral::Property* property) { Files seems to be missing from the code review. This changes the signature, but parser.h is not included. You need a good comment in parser.h to explain the return values of this method. http://codereview.chromium.org/39184/diff/1/6#newcode3212 Line 3212: literal_index, Do you still allocate all the nested literals in the literals array? Doesn't that mean that you get two identical boilerplates for the same nested literal? One in the object literal in which it is nested and one in the list of literals in the function? If that is the case, we should make sure to avoid that duplication. http://codereview.chromium.org/39184/diff/1/4 File src/runtime.cc (right): http://codereview.chromium.org/39184/diff/1/4#newcode135 Line 135: Handle<FixedArray> literals, Indentation is off for these three lines. http://codereview.chromium.org/39184/diff/1/4#newcode161 Line 161: if (value->IsFixedArray()) { The use of fixed arrays to signal that this was a nested object literal needs a comment. http://codereview.chromium.org/39184/diff/1/4#newcode163 Line 163: value = CreateObjectLiteralBoilerplate(literals, -1, array); It might make the code more readable to introduce a named constant for -1? Or it might be even better to have two functions. One for constructing an object literal boilerplate and one for constructing a nested object literal boilerplate. They can share all the code in a helper function but one of them adds the final boilerplate to the boilerplate array. http://codereview.chromium.org/39184 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
