LGTM.
http://codereview.chromium.org/3595013/diff/1/2 File src/handles.cc (right): http://codereview.chromium.org/3595013/diff/1/2#newcode638 src/handles.cc:638: static void CheckKeys(Handle<FixedArray> array) { Maybe turns this into a function that returns a bool and call it like: ASSERT(ContainsOnlyValidKeys(array)) and let the implementation be valid in release mode too. Less ifdef'ing makes me happy. http://codereview.chromium.org/3595013/diff/1/3 File src/objects.cc (right): http://codereview.chromium.org/3595013/diff/1/3#newcode3606 src/objects.cc:3606: // We cannot optimize if this is empty, as other my have holes Maybe quote 'this'. Maybe explain why 'this' cannot have holes. my => may not keys => non keys? http://codereview.chromium.org/3595013/diff/1/4 File test/cctest/test-api.cc (right): http://codereview.chromium.org/3595013/diff/1/4#newcode11486 test/cctest/test-api.cc:11486: Two newlines between the functions definitions. http://codereview.chromium.org/3595013/diff/1/4#newcode11487 test/cctest/test-api.cc:11487: v8::Handle<v8::Value> Getter(v8::Local<v8::String> property, Could this be static? You don't need access to it from other .cc files. http://codereview.chromium.org/3595013/diff/1/4#newcode11489 test/cctest/test-api.cc:11489: return String::New("42!"); Can't you use v8_str here? http://codereview.chromium.org/3595013/diff/1/4#newcode11492 test/cctest/test-api.cc:11492: v8::Handle<v8::Array> Enumerator(const v8::AccessorInfo& info) { Could this be static? You don't need access to it from other .cc files. http://codereview.chromium.org/3595013/diff/1/4#newcode11494 test/cctest/test-api.cc:11494: result->Set(0, String::New("universalAnswer")); Can't you use v8_str here? http://codereview.chromium.org/3595013/diff/1/4#newcode11500 test/cctest/test-api.cc:11500: v8::Persistent<v8::Context> context = Context::New(); Can you use a stack-allocated LocalContext instead of creating a persistent handle that you never destroy? If not, you should remember to clean up by destroying the persistent handle. http://codereview.chromium.org/3595013/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
