An initial set of comments below. I will now look at the newly uploaded revision.
-Ivan http://codereview.chromium.org/7990/diff/1/6 File src/array.js (right): http://codereview.chromium.org/7990/diff/1/6#newcode377 Line 377: var args = $Array(1 + arg_count); After consulting the spec I see why this works, but it is very, very, very confusing. Please use "new $Array" instead. http://codereview.chromium.org/7990/diff/1/6#newcode378 Line 378: var result = $Array(); ditto http://codereview.chromium.org/7990/diff/1/4 File src/runtime.cc (right): http://codereview.chromium.org/7990/diff/1/4#newcode3990 Line 3990: class ArrayConcatVisitor { Comments please. http://codereview.chromium.org/7990/diff/1/4#newcode4014 Line 4014: static uint32_t VisitElements(Handle<JSObject> receiver, This function should probably be renamed IterateArrayElements() to avoid confusion with a real Visitor. Also please add more general comments about this function please. http://codereview.chromium.org/7990/diff/1/4#newcode4058 Line 4058: static uint32_t VisitArrayElements(Handle<JSArray> array, This function should probably be renamed IterateArrayElements. http://codereview.chromium.org/7990/diff/1/4#newcode4065 Line 4065: List<Handle<JSObject> > objects(kEstimatedPrototypes); Extra space in "List<Handle<JSObject> >"? Slightly confusing as it tends to read as a "greater than" operator. http://codereview.chromium.org/7990/diff/1/4#newcode4067 Line 4067: // Visit prototype first. You should comment this whole recursion avoidance by adding to an array and iterating it backwards a bit better. http://codereview.chromium.org/7990/diff/1/4#newcode4084 Line 4084: static uint32_t VisitArrays(Handle<JSArray> arrays, This should probably be renamed to IterateArguments, because you really iterate over the arguments being passed to Array.Concat. Also please adjust the internal variable names accordingly. http://codereview.chromium.org/7990/diff/1/4#newcode4140 Line 4140: uint32_t estimate_nof_elements = VisitArrays(arrays, 0); 0 should probably be NULL http://codereview.chromium.org/7990 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
