LGTM with comments.
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) { 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 :). http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode505 samples/shell.cc:505: if (args[0]->Int32Value() < 0) { You convert args[0] to int32 twice. Just do it once and save the value ... except that I don't think you want the int32 value in this check. Instead, do a proper overflow check on the value (if someone passes in a double number with value kMaxLength + 2 * kMaxInt, it should be rejected, not silently converted to kMaxLength-2). 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)) { If both values are int32, there's no need to convert them both to size_t before checking. 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). http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode518 samples/shell.cc:518: v8::String::New("Array size exceeds memory limit.")); Can this happen? I.e., is kMaxLength * maximal element size ever not representable as a size_t. Otherwise, just make an ASSERT. http://codereview.chromium.org/7268002/diff/1/samples/shell.cc#newcode520 samples/shell.cc:520: void* data = malloc(malloc_size); How about using calloc instead? It seems like just the thing for allocating arrays, and it even initializes the memory to zero. http://codereview.chromium.org/7268002/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
