https://chromiumcodereview.appspot.com/15001041/diff/7001/include/v8.h
File include/v8.h (right):
https://chromiumcodereview.appspot.com/15001041/diff/7001/include/v8.h#newcode2342
include/v8.h:2342: ArrayBufferContents() : data_(NULL), byte_length_(0)
{}
Nit: We usually put the constructor/destructor at the beginning of the
corresponding public/protected/private section.
https://chromiumcodereview.appspot.com/15001041/diff/7001/include/v8.h#newcode2348
include/v8.h:2348: { }
Nit: Put this at the end of the initializer list.
https://chromiumcodereview.appspot.com/15001041/diff/7001/src/api.cc
File src/api.cc (right):
https://chromiumcodereview.appspot.com/15001041/diff/7001/src/api.cc#newcode6023
src/api.cc:6023: i::Isolate* isolate =
Utils::OpenHandle(this)->GetIsolate();
Just remove this line and the following IsDeadCheck. This idiom is
fundamentally broken, anyway, and we want to remove it completely in the
future, so no need for it in new code.
https://chromiumcodereview.appspot.com/15001041/diff/7001/src/api.cc#newcode6025
src/api.cc:6025: i::Handle<i::JSArrayBuffer> obj =
Utils::OpenHandle(this);
Nit: No need to name this temporary.
https://chromiumcodereview.appspot.com/15001041/diff/7001/src/api.cc#newcode6030
src/api.cc:6030: if (data_ != NULL) {
No need for this guard, free(NULL) is by definition a no-op.
https://chromiumcodereview.appspot.com/15001041/diff/7001/src/api.cc#newcode6039
src/api.cc:6039: i::Isolate* isolate =
Utils::OpenHandle(this)->GetIsolate();
Kill the IsDeadCheck here, too.
https://chromiumcodereview.appspot.com/15001041/diff/7001/src/api.cc#newcode6047
src/api.cc:6047: contents->data_ = obj->backing_store();
We silently overwrite any previous backing store in contents here. This
part of the API looks very fragile and will easily lead to programming
errors. Can we do this differently?
https://chromiumcodereview.appspot.com/15001041/diff/7001/src/d8.cc
File src/d8.cc (right):
https://chromiumcodereview.appspot.com/15001041/diff/7001/src/d8.cc#newcode1080
src/d8.cc:1080: weak_handle.MarkIndependent(isolate);
IIRC, dcarney wants to move to a non-Isolate version in the long run, so
we should remove parameter here.
https://chromiumcodereview.appspot.com/15001041/diff/7001/src/runtime.cc
File src/runtime.cc (right):
https://chromiumcodereview.appspot.com/15001041/diff/7001/src/runtime.cc#newcode668
src/runtime.cc:668: if (data != NULL)
No need for this guard here, either.
https://chromiumcodereview.appspot.com/15001041/diff/7001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):
https://chromiumcodereview.appspot.com/15001041/diff/7001/test/cctest/test-api.cc#newcode2496
test/cctest/test-api.cc:2496: THREADED_TEST(ArrayBuffer) {
Can we please split this test into several tests? Currently it is very
hard to see what the various parts are actually testing. Furthermore, do
we have simple test to see what happens on the JavaScript side when an
ArrayBuffer moves to the "externalized" state? If there is one, I can't
see it...
https://chromiumcodereview.appspot.com/15001041/diff/7001/test/cctest/test-api.cc#newcode15477
test/cctest/test-api.cc:15477: ElementType* backing_store = new
ElementType[kElementCount+2];
Using 'ScopedVector<ElementType> backing_store(kElementCount+2)'
simplifies things here.
https://chromiumcodereview.appspot.com/15001041/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
