Please take another look at this.
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_; On 2009/03/05 13:19:14, Mads Ager wrote: > 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)? I went with 'simple'. 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) { On 2009/03/05 13:19:14, Mads Ager wrote: > 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. The Parser class is defined in the parser.cc file, not parser.h. I've amended the comment for GetBoilerplateValue above. http://codereview.chromium.org/39184/diff/1/6#newcode3212 Line 3212: literal_index, On 2009/03/05 13:19:14, Mads Ager wrote: > 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. I don't create code for the nested literal, so I don't get two identical boilerplates. I still reserve a space for them in the literals array, though. I don't think saving that space is worth the hassle of figuring whether we'll need it. 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, On 2009/03/05 13:19:14, Mads Ager wrote: > Indentation is off for these three lines. Fixed. Also fixed for the function above this. http://codereview.chromium.org/39184/diff/1/4#newcode161 Line 161: if (value->IsFixedArray()) { On 2009/03/05 13:19:14, Mads Ager wrote: > The use of fixed arrays to signal that this was a nested object literal needs a > comment. Comment added. http://codereview.chromium.org/39184/diff/1/4#newcode163 Line 163: value = CreateObjectLiteralBoilerplate(literals, -1, array); On 2009/03/05 13:19:14, Mads Ager wrote: > 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. Removed the literal_index from the recursion. http://codereview.chromium.org/39184 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
