Do not shortcut union of keys if lhs is empty.

Do not shortcut union of keys if lhs is empty.

The problem is other array may have holes, for example
when fixed array comes from JSArray (in case of named interceptor).

If that would prove to be a performance problem, we could
pass an additional argument into UnionOfKeys to hold actual length.

Committed: http://code.google.com/p/v8/source/detail?r=5591

[antonm, 2010-10-05 12:26:28]

Kasper,

may you have a look?

[Kasper Lund, 2010-10-05 12:39:12]

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.

[antonm, 2010-10-05 13:09:58]

Thanks a lot for review, Kasper.  Submitting.

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) {
On 2010/10/05 12:39:12, Kasper Lund wrote:
> 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.

Done.  That requires USE for Release mode though

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
On 2010/10/05 12:39:12, Kasper Lund wrote:
> Maybe quote 'this'. Maybe explain why 'this' cannot have holes.
> 
> my => may
> not keys => non keys?

Almost done.  I failed to find proper explanation why this shouldn't contain any
holes and just added another check, even though it is somewhat redundant.

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: 
On 2010/10/05 12:39:12, Kasper Lund wrote:
> Two newlines between the functions definitions.

Sorry, all addressed, haven't done enough style adjustment to foreign code, mea
culpa.