From 147806b39521cafa377a12bb1eb225f25be97245 Mon Sep 17 00:00:00 2001 From: Alex Robinson Date: Thu, 6 Oct 2022 10:09:49 -0500 Subject: [PATCH] Remove ArrayPtr from list of supported jsg wrapping types It does not work when I try to add an ArrayPtr member field to a JSG_STRUCT, so we should remove the documentation of it. The functionality appears to have been removed in ca17240f28279292f5fa8ae281965ee982a9cac7 without updating the comments. --- src/workerd/jsg/jsg.h | 1 - src/workerd/jsg/value.h | 18 +++++++----------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/workerd/jsg/jsg.h b/src/workerd/jsg/jsg.h index 5401783ee77..805eaac0e81 100644 --- a/src/workerd/jsg/jsg.h +++ b/src/workerd/jsg/jsg.h @@ -223,7 +223,6 @@ class JsExceptionThrown: public std::exception { // - C++ kj::OneOf <-> JS T or U or ... // - C++ kj::Array <-> JS Array of T // - C++ kj::Array <-> JS ArrayBuffer -// - C++ kj::ArrayPtr <- JS ArrayBuffer, ArrayBufferView // - C++ jsg::Dict <-> JS Object used as a map of strings to values of type T // - C++ jsg::Function <-> JS Function // - C++ jsg::Promise <-> JS Promise diff --git a/src/workerd/jsg/value.h b/src/workerd/jsg/value.h index 0f2d588bab4..db3cc7f28a7 100644 --- a/src/workerd/jsg/value.h +++ b/src/workerd/jsg/value.h @@ -843,20 +843,16 @@ class ArrayWrapper { // (the kj::Array object holds a Global to the unwrapped ArrayBuffer) // - ArrayBufferView -> kj::Array // (the kj::Array object holds a Global to the unwrapped ArrayBufferView's backing buffer) -// - ArrayBuffer -> kj::ArrayPtr -// - ArrayBufferView -> kj::ArrayPtr // -// Note that there is no wrapping conversion from kj::ArrayPtr, since it does not own its -// own buffer -- fine in C++, but problematic in a GC language like JS. Since the JS object would -// need to hold shared ownership of its buffer, but we don't know who owns the buffer viewed by an -// ArrayPtr, we would need to create a copy of the buffer. You should probably make that copy -// explicitly with a kj::heapArray() call, meaning we only need to wrap kj::ArrayPtrs. +// Note that there are no conversions for kj::ArrayPtr, since it does not own its own +// buffer -- fine in C++, but problematic in a GC language like JS. Restricting the interface to +// only operate on owned arrays makes memory management simpler and safer in both directions. // // Logically a kj::Array could be considered analogous to a Uint8Array in JS, and for a time // that was the wrapping conversion implemented by this wrapper. However, the most common use cases // in web platform APIs involve accepting BufferSources for processing as immutable input and // returning ArrayBuffers. Since a kj::byte does not map to any JavaScript primitive, establishing -// a mapping between ArrayBuffer/ArrayBufferView and Array/ArrayPtr is unambiguous and +// a mapping between ArrayBuffer/ArrayBufferView and Array is unambiguous and // convenient. The few places where a specific TypedArray is expected (e.g. Uint8Array) can be // handled explicitly with a v8::Local (or other appropriate TypedArray type). // @@ -870,13 +866,13 @@ class ArrayWrapper { // This suggests the following rules of thumb: // // 1. If a BufferSource parameter is used as input to a: -// - synchronous method: accept a `kj::ArrayPtr`. +// - synchronous method: accept a `kj::Array`. // - asynchronous method (user is allowed to re-use the buffer during processing): accept a -// `kj::ArrayPtr` and explicitly copy its bytes. +// `kj::Array` and explicitly copy its bytes. // // 2. If a method accepts an ArrayBufferView that it is expected to mutate: // - accept a `v8::Local` explicitly (handled by V8HandleWrapper in -// type-wrapper.h) rather than a `kj::ArrayPtr` -- otherwise your method's contract +// type-wrapper.h) rather than a `kj::Array` -- otherwise your method's contract // will be wider than intended. // - use `jsg::asBytes()` as a quick way to get a `kj::ArrayPtr` view onto it. //