Removing a wrong check....

Removing a wrong check.

A strings which represents an array index with length 8 and 9 digits do not pass this check. However generated hash is valid.


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

[caseq, 2010-09-06 17:56:32]

On 2010/09/06 17:15:48, SeRya wrote:
> 

Sergey, please have someone from v8 team review this -- I'm not familiar with
this code (actually, removing this ASSERT looks a bit fishy to me, and check for
is_array_index() that was in StringHasher::MakeCachedArrayIndex() previously
seemed to make sense).

Also, in StringHasher::MakeCachedArrayIndex(),
ASSERT(String::kMaxArrayIndexSize < (1 << String::kArrayIndexValueBits));
seems suspicious (though it's been there before your change) -- we're comparing
number of decimal digits in array index (10) against maximum value of array
index. Shouldn't this be 
ASSERT(TenToThe(String::kMaxArrayIndexSize) < (1 <<
String::kArrayIndexValueBits))?

[antonm, 2010-09-06 19:10:58]

Drive by comments.

I agree with Andrei (caseq@) that check in MakeCachedArrayIndex seems strange. 
But if understand what goes on here correctly, ASSERT(length <=
String::kMaxArrayIndexSize); is fishy as well.

If I am right, we want something like:

ASSERT(length > 0);
ASSERT(length <= String::kMax[Cached]ArrayIndex[Length]);
ASSERT(TenToThe(String::kMaxCachedArrayIndexLength) <
    (1 << String::kArrayIndexValueBits));
ASSERT(String::kMaxArrayIndexSize >= String::kMaxCachedArrayIndex);

But the last check only checks we don't spend unnecessary stuff.

Plus probably most of those checks should be turned into STATIC_ASSERT and moved
into String class itself.

http://codereview.chromium.org/3295017/diff/1/2
File src/runtime.cc (left):

http://codereview.chromium.org/3295017/diff/1/2#oldcode4858
src/runtime.cc:4858: len <= String::kMaxArrayIndexSize &&
I think a mistake is here: it should be len <=
String::kMaxCachedArrayIndexLength----we only have 24 bits for an index which
only allows room for 10 ** 7 (but my knowledge of this part of v8 is poor).

If I am right, we need to go over doc and fix it.

[SeRya, 2010-09-07 09:38:02]

The disscussed assert removed from MakeCachedArrayIndex, added STATIC_CHECK to
objects.h.

[Vyacheslav Egorov (Chromium), 2010-09-07 10:21:58]

LGTM

if you add 

ASSERT((length > kMaxCachedArrayIndexLength) ||
       (hash & String::kContainsCachedArrayIndexMask) == 0);

into MakeCachedArrayIndex and rename it per Anton's suggestion into
MakeArrayIndexHash.

[SeRya, 2010-09-07 10:49:20]

The assert added, the function renamed.

[Vyacheslav Egorov (Chromium), 2010-09-07 10:54:53]

still LGTM.

[antonm, 2010-09-07 11:32:05]

LGTM and thanks