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
