I think ASSERT(String::kMaxArrayIndexSize < (1 << String::kArrayIndexValueBits));
was originally intended to be ASSERT(String::kMaxArrayIndexSize < (1 << String::kArrayIndexLengthBits)); I think I mistyped this when I was refactoring this place (r5363). In any case this should be better turned into STATIC_ASSERT and moved to right into class String. >> But if understand what goes on here correctly, ASSERT(length <= >> String::kMaxArrayIndexSize); is fishy as well. It is fishy but it is correct: we encode length into hash field even if it bigger than kMaxCachedArrayIndexLength so we can later do a fast check through kContainsCachedArrayIndexMask. Instead of removing assertion ASSERT((hash & String::kContainsCachedArrayIndexMask) == 0); I recommend transforming it and moving into MakeCachedArrayIndex: ASSERT((length > kMaxCachedArrayIndexLength) || (hash & String::kContainsCachedArrayIndexMask) == 0); This will check that mask correctly detects cases of length > kMaxCachedArrayIndexLength. -- Vyacheslav Egorov On Mon, Sep 6, 2010 at 9:10 PM, <[email protected]> wrote: > 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. > > http://codereview.chromium.org/3295017/show > > -- > v8-dev mailing list > [email protected] > http://groups.google.com/group/v8-dev > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
