Thanks for the review. I've addressed your comments.
If there's anything else, I'm happy to discuss this in person to reduce
latency
and increase interactiveness :-)
http://codereview.chromium.org/7268002/diff/1/samples/shell.cc
File samples/shell.cc (right):
http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode500
samples/shell.cc:500: size_t element_size) {
On 2011/06/28 09:28:58, Lasse Reichstein wrote:
Let's ASSERT that element_size it's a meaningful value (like, always a
positive
power of two and no bigger than 8).
In which case you don't need to change the type in the interface
(although I
personally support using size_t for sizes everywhere :).
Done.
Changed the interface to size_t anyway because it's more consistent.
http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode505
samples/shell.cc:505: if (args[0]->Int32Value() < 0) {
On 2011/06/28 09:44:12, Lasse Reichstein wrote:
That is, ofcourse, unless there is a specification that says to
convert args[0]
using ToInt32. In that case, just do it once.
Spec says the c'tor argument is an "unsigned long", so I think ToInt32
should be fine, considering the v8-internal maximum value we check
against anyway (see below).
Converting only once: done.
http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode510
samples/shell.cc:510: if (length >
static_cast<size_t>(v8::internal::ExternalArray::kMaxLength)) {
On 2011/06/28 09:28:58, Lasse Reichstein wrote:
If both values are int32, there's no need to convert them both to
size_t before
checking.
True. Removed the cast.
Is the kMaxLength independent of element size? I would have expected a
kMaxSize
(max memory use) and a max length derived from that (by subtracting
any overhead
and dividing by element size).
Nope, kMaxLength is type independent.
http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode518
samples/shell.cc:518: v8::String::New("Array size exceeds memory
limit."));
On 2011/06/28 09:28:58, Lasse Reichstein wrote:
Can this happen?
I.e., is kMaxLength * maximal element size ever not representable as a
size_t.
Otherwise, just make an ASSERT.
It can happen. kMaxLength is 2^30 - 1, and Float64Array's element_size
is 8, resulting in 8GB theoretical max size, which a 32bit VM obviously
won't let us allocate.
However, by using calloc() below, this manual check becomes superfluous.
http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode520
samples/shell.cc:520: void* data = malloc(malloc_size);
On 2011/06/28 09:28:58, Lasse Reichstein wrote:
How about using calloc instead? It seems like just the thing for
allocating
arrays, and it even initializes the memory to zero.
Done, thanks for the hint.
http://codereview.chromium.org/7268002/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
