Fix an error in a keyed lookup stub - HeapNumbers treated as strings.

Fix an error in a keyed lookup stub - HeapNumbers treated as strings.

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

[iposva, 2009-07-22 18:59:55]

-Ivan

http://codereview.chromium.org/155924/diff/3/1008
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/155924/diff/3/1008#newcode51
Line 51: // complete runtime version.
Bill since we only ever enter symbols into the dictionaries as keys executing
the code generated by this function with a non-symbol string is a waste. Please
update the comment to this extent.

http://codereview.chromium.org/155924/diff/3/1008#newcode286
Line 286: // Is it a string?
Excellent catch!

http://codereview.chromium.org/155924/diff/3/1008#newcode297
Line 297: // and separately for strings with a computed hash and those without.
Please keep the filter for symbols here as it is a waste to execute the
dictionary load with a non-symbol string.

[William Hesse, 2009-07-23 08:55:58]

http://codereview.chromium.org/155924/diff/3/1008
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/155924/diff/3/1008#newcode51
Line 51: // complete runtime version.
On 2009/07/22 18:59:55, iposva wrote:
> Bill since we only ever enter symbols into the dictionaries as keys executing
> the code generated by this function with a non-symbol string is a waste.
Please
> update the comment to this extent.

Done.

http://codereview.chromium.org/155924/diff/3/1008#newcode297
Line 297: // and separately for strings with a computed hash and those without.
On 2009/07/22 18:59:55, iposva wrote:
> Please keep the filter for symbols here as it is a waste to execute the
> dictionary load with a non-symbol string.

Done.

[Mads Ager (chromium), 2009-07-23 11:41:51]

I would prefer to keep the old style check instead of forcing a use of
CmpInstanceType.

http://codereview.chromium.org/155924/diff/2003/1016
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/155924/diff/2003/1016#newcode294
Line 294: __ CmpInstanceType(edx, FIRST_SYMBOL_TYPE);
I find this misleading since FIRST_SYMBOL_TYPE is actually not an instance type
at all.  I like the explict checking of kSymbolMask better.

http://codereview.chromium.org/155924/diff/2003/1018
File src/objects.h (right):

http://codereview.chromium.org/155924/diff/2003/1018#newcode699
Line 699: FIRST_SYMBOL_TYPE = kSymbolTag
This is not an instance type, so I find it misleading to have it here.  Can't
you write the code using kSymbolTag explicitly?

[William Hesse, 2009-07-23 12:53:07]

http://codereview.chromium.org/155924/diff/2003/1016
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/155924/diff/2003/1016#newcode294
Line 294: __ CmpInstanceType(edx, FIRST_SYMBOL_TYPE);
On 2009/07/23 11:41:51, Mads Ager wrote:
> I find this misleading since FIRST_SYMBOL_TYPE is actually not an instance
type
> at all.  I like the explict checking of kSymbolMask better.

Done.

http://codereview.chromium.org/155924/diff/2003/1018
File src/objects.h (right):

http://codereview.chromium.org/155924/diff/2003/1018#newcode699
Line 699: FIRST_SYMBOL_TYPE = kSymbolTag
On 2009/07/23 11:41:51, Mads Ager wrote:
> This is not an instance type, so I find it misleading to have it here.  Can't
> you write the code using kSymbolTag explicitly?

Done.

[Mads Ager (chromium), 2009-07-23 12:57:52]

LGTM, thanks!