Typed arrays are necessary to pass chunks of data i.e. a byte buffer as Uint8Array between the C/C++ and JavaScript worlds. Using strings is not feasible for many uses because where the representation of the data in memory must be the same, bit-by-bit, on both sides.
Created attachment 368836 [details] WIP Patch v1 Here goes an initial version of the patch with the proposed API additions. It still needs API tests and documentation, at least.
Attachment 368836 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1475: jsc_value_typed_array__from_data is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1481: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1488: jsc_value_is_typed_array is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1502: jsc_value_typed_array_get_length is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1520: jsc_value_typed_array_get_contents is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCValue.h" Total errors found: 5 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
The following API are missing documentation: JSCTypedArrayType jsc_value_is_typed_array jsc_value_typed_array_get_contents jsc_value_typed_array_get_length jsc_value_typed_array_get_type jsc_value_typed_array_new jsc_value_typed_array_new_from_data
Comment on attachment 368836 [details] WIP Patch v1 Attachment 368836 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12069414 New failing tests: security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html
Created attachment 368852 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
(In reply to Michael Catanzaro from comment #3) > The following API are missing documentation: > JSCTypedArrayType > jsc_value_is_typed_array > jsc_value_typed_array_get_contents > jsc_value_typed_array_get_length > jsc_value_typed_array_get_type > jsc_value_typed_array_new > jsc_value_typed_array_new_from_data Of course, I am perfectly aware; I just wanted to share an initial WIP version of the patch to get the discussion started about the new API and avoid writing documentation for functions which might get changed or removed before being rubber-stamped ;-) Now, regarding the API, reviewers may be wondering why GBytes, GArray and/or GByteArray are *not* used. The reason is that their semantics do not match typed arrays well: - Typed arrays are fixed size, which discards usage of GArray and GByteArray. These GLib types allow the backing memory buffer to be resized e.g. via g_array_append_val() or g_byte_array_set_size(). - While GBytes seems like a good match on the surface, the expectation is that the contents of the backing memory buffer are immutable. This does not hold true for a JS typed array, where individual elements can be mutated. Because of the reasons above, I settled on having _new_from_data() take a pointer+size pair, and an optional GDestroyNotify. On the other end of the spectrum, a raw pointer and the size of the backing buffer in bytes can be obtained directly instead of having those wrapped into some helper object — like for GMappedFile, which I presume has the same rationale behind its _get_length() and _get_contents() methods. Last but not least, it will be important to note that the pointer returned by _get_contents() may become invalid between JSC API calls because JSC might change the underlying array buffer under the hood. This will be something to stress in the API reference documentation.
Comment on attachment 368836 [details] WIP Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=368836&action=review > Source/JavaScriptCore/API/glib/JSCValue.cpp:1520 > +char* jsc_value_typed_array_get_contents(JSCValue* value, gsize* len) Use guchar* to indicate that it's not a string. (char*/gchar* is guaranteed to be a UTF-8 string in GObject APIs.)
(In reply to Michael Catanzaro from comment #7) > Comment on attachment 368836 [details] > WIP Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=368836&action=review > > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1520 > > +char* jsc_value_typed_array_get_contents(JSCValue* value, gsize* len) > > Use guchar* to indicate that it's not a string. > > (char*/gchar* is guaranteed to be a UTF-8 string in GObject APIs.) Ah, this is a good point, let's change the types to guchar* =) I have been testing out the new API myself and in general it works nicely. The only modification I think that would be good to have is adding an “user_data” parameter to the function that creates a new typed array from an existing data buffer, like the following: JSCValue* jsc_value_new_typed_array_from_data (JSCContext *context, JSCTypedArrayType type, gpointer data, gsize data_bytes, GDestroyNotify destroy_notify, gpointer user_data); With this modification, what would be passed to the “destroy_notify” callback would be “user_data”, instead of “data”. This is desirable because the “data” buffer might belong to some other structure — note that passing the same value for “data” and “user_data” would still achieve the goal of passing “data” to the “destroy_notify” callback. If you want a concrete example where “user_data” actually needs to be different, consider this example (sans error checking, to keep it brief): JSCValue* js_map_file (const char* file_path) { GMappedFile *f = g_mapped_file_new (file_path, TRUE, NULL); return jsc_value_new_typed_array_from_data (jsc_context_get_current (), JSC_TYPED_ARRAY_UINT8, g_mapped_file_get_contents (f), g_mapped_file_get_length (f), (GDestroyNotify) g_mapped_file_unref, f); } Note how we do *not* want the “destroy_notify” to be applied to the data buffer pointer; in this case we want to apply “g_mapped_file_unref” on the GMappedFile itself instead. I will do this change, which is important for many use cases, then add tests/documentation, and post an updated patch :-)
Created attachment 369379 [details] WIP Patch v2 This version of the patch has the following changes from v1: - New API functions and types now have documentation. - jsc_typed_array_new_from_data() now can take an “user_data”, which is what gets passed to the “destroy_notify” callback. This provides some needed flexibility to allow wrapping something more complex than a raw memory buffer as a typed array (check the documentation, which uses a GMappedFile to exemplify this). - jsc_typed_array_get_contents() now returns “gpointer”. I think this is better than the “guchar*” suggestion by Michael, because it forces the users of the function to think and deliberately choose which pointer type they need to cast the returned value to. This is good because typed array contents can be elements of different types, and forcing users to make an explicit choice avoids a potential footgun. The patch still needs adding unit tests for the new API, which is the next thing I will be doing. Any feedback that reviewers can provide in the meantimea bout the patch will be valuable (thanks!)
Attachment 369379 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1493: jsc_value__typed_array is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1554: jsc_value__typed_array_from_data is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1584: jsc_value_is_typed_array is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1608: jsc_value_typed_array_get_length is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/API/glib/JSCValue.cpp:1650: jsc_value_typed_array_get_contents is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] WARNING: File exempt from style guide. Skipping: "Source/JavaScriptCore/API/glib/JSCValue.h" Total errors found: 5 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 369379 [details] WIP Patch v2 Attachment 369379 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12141767 New failing tests: http/tests/css/filters-on-iframes.html
Created attachment 369480 [details] Archive of layout-test-results from ews213 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews213 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 369379 [details] WIP Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=369379&action=review > Source/JavaScriptCore/API/glib/JSCValue.cpp:1484 > + * @len: number of elements in the array length > Source/JavaScriptCore/API/glib/JSCValue.cpp:1554 > +JSCValue* jsc_value_new_typed_array_from_data(JSCContext* context, JSCTypedArrayType type, gpointer data, gsize dataBytes, gpointer userData, GDestroyNotify destroyNotify) I wonder if we should expose ArrayBuffer (we probably need to do it anyway) and then leave the destruction thing to array buffer itself, and expose only new_typed_array_from_array_buffer (not this data one). > Source/JavaScriptCore/API/glib/JSCValue.cpp:1557 > + Is it possible to create a typed array with null data? and 0 size then? > Source/JavaScriptCore/API/glib/JSCValue.cpp:1560 > + deallocatorContext = static_cast<TypedArrayDeallocatorContext*>(fastMalloc(sizeof(TypedArrayDeallocatorContext))); You can use WEBKIT_DEFINE_ASYNC_DATA_STRUCT macro. > Source/JavaScriptCore/API/glib/JSCValue.cpp:1602 > + * Obtains the size in bytes of the memory region which holds the contents of a typed array. This is confusing, I would expect the lenght to be the number of elements in the array, not the data size in bytes. > Source/JavaScriptCore/API/glib/JSCValue.cpp:1604 > + * Returns: size in bytes or zero if @value is not a typed array Wouldn't we return 0 also if the array is empty? > Source/JavaScriptCore/API/glib/JSCValue.cpp:1619 > + size_t len = JSObjectGetTypedArrayLength(jsContext, jsObject, &exception); This is indeed the amount of elements in array. > Source/JavaScriptCore/API/glib/JSCValue.cpp:1627 > + * jsc_value_typed_array_get_contents: If we expose array buffer, this would be part of the array buffer api. I think it's would be less confusing, because typed array woudl always be about elements and array buffer about the actual data and size in bytes. Note also that is possible to provide an offset. > Source/JavaScriptCore/API/glib/JSCValue.cpp:1629 > + * @len: (nullable): if not `NULL`, location of a variable where to store the length in bytes length. %NULL
I've been checking the patch because we found some crashes in a demo using this API. The memory we are passing to WebKit can not be reserved outside with malloc if WebKit is using gigacage. I don't know much about gigacage but it seems a solution would be to reserve memory inside gigacage and copy the data passed to the jsc_value_new_typed_array_from_data API. But it is true that if the whole point of the API is to adopt the memory we have to change the name to add the copy somewhere in the name to make it clear. Also we should add an assert or some way to control this situation and fail nicely with some informative message to the developer. I would not even propose a solution in case gigacage is not activated to make sure no one tries to do it because it performs better. I hope it helps!
(In reply to Alejandro G. Castro from comment #14) > I've been checking the patch because we found some crashes in a demo using > this API. The memory we are passing to WebKit can not be reserved outside > with malloc if WebKit is using gigacage. I don't know much about gigacage > but it seems a solution would be to reserve memory inside gigacage and copy > the data passed to the jsc_value_new_typed_array_from_data API. But it is > true that if the whole point of the API is to adopt the memory we have to > change the name to add the copy somewhere in the name to make it clear. > > Also we should add an assert or some way to control this situation and fail > nicely with some informative message to the developer. > > I would not even propose a solution in case gigacage is not activated to > make sure no one tries to do it because it performs better. > > I hope it helps! Thanks Alex, that indeed is useful information and makes complete sense. I completely agree that we do not want to give any hint that would suggest disabling gigacages as an option because it's an important safety feature. I am going to restart working on this now, and I will try to come up with a better API that exposes ArrayBuffer as suggested by Carlos (which can be useful in many cases, like using Blob.arrayBuffer when a C function gets a Blob instance passed to it) and also some way of asking JSC itself to allocate a chunk of memory so it can be inside a suitable gigacage, and returning that via the JSC API so client code can put data in buffers that JSC can then use as ArrayBuffer backing. That will be probably returning GBytes instances that wrap the memory allocated by JSC and have a custom deallocation callback that takes care of properly freeing memory.
I am splitting the part of exposing ArrayBuffer in the public API to bug #237088, and then adding functions for typed arrays will build upon that.
Created attachment 454040 [details] WIP Patch v3 This version of the patch builds upon the one from bug #237088, and I think it should address all the concerns that Carlos Garcia had. As for the allocation issue mentionex by Alex, it seems that creating an ArrayBuffer first, and then using it as the memory area backing a typed array works as expected even with non-gigacage allocations (which IMO makes sense). I will be uploading a version of the patch with added API tests, but if anybody has feedback in the meantime, it would be appreciated.
Created attachment 454112 [details] Patch Now with API tests. I am quite satisfied with how the API turned out, and it has definitely been a good idea to add support for ArrayBuffer as suggested ;-)
Of course the EWS will be red because we need to land the patch from bug #237088 first.
Comment on attachment 454112 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454112&action=review > Source/JavaScriptCore/API/glib/JSCValue.cpp:1677 > + case kJSTypedArrayTypeNone: > + case kJSTypedArrayTypeArrayBuffer: return JSC_TYPED_ARRAY_NONE; I think this is easier to read if returns are in new lines. At first I thought you had forgotten to add a return for the None, then I realized we return NONE also for ArrayBuffer. Again, I think the C API is confusing, we should either add a comment explaining what happens with the ArrayBuffer type, or not use JSValueGetTypedArrayType and simply check toJSTypedArrayType(object->classInfo(vm)->typedArrayStorageType) directly > Source/JavaScriptCore/API/glib/JSCValue.cpp:1705 > + * The @type must *not* be #JSC_TYPED_ARRAY_NONE. #JSC_TYPED_ARRAY_NONE -> %JSC_TYPED_ARRAY_NONE > Source/JavaScriptCore/API/glib/JSCValue.cpp:1714 > + g_return_val_if_fail(JSC_IS_CONTEXT(context), nullptr); > + g_return_val_if_fail(type != JSC_TYPED_ARRAY_NONE, nullptr); > Source/JavaScriptCore/API/glib/JSCValue.cpp:1742 > +JSCValue* jsc_value_new_typed_array_with_buffer(JSCValue* arrayBuffer, JSCTypedArrayType type) I think we want to expose the constructor taking an offset too. The JS API allows to pass only a buffer, buffer + offset and buffer + offset + length. Adding 3 constructors would be too much so we can probably leave this and add with_offset taking both offset and length, but length being optional, or simply add offset and length here, making both optional. > Source/JavaScriptCore/API/glib/JSCValue.cpp:1783 > + return type != kJSTypedArrayTypeNone && type != kJSTypedArrayTypeArrayBuffer; This is confusing too. > Source/JavaScriptCore/API/glib/JSCValue.cpp:1850 > + auto* ptr = JSObjectGetTypedArrayBytesPtr(jsContext, jsObject, &exception); ptr -> bytes, data or something like that > Source/JavaScriptCore/API/glib/JSCValue.cpp:1860 > + const auto c = JSObjectGetTypedArrayLength(jsContext, jsObject, &exception); c -> length, size or something like that > Source/JavaScriptCore/API/glib/JSCValue.cpp:1870 > + * jsc_value_typed_array_get_count: I would use length instead of count > Source/JavaScriptCore/API/glib/JSCValue.cpp:1899 > + * Obtain the %ArrayBuffer for the memory region of the typed array elements. I guess this returns an array buffer even when the types array was not created with a buffer, right? I would document it and add a test case if there isn't. > Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3039 > + g_assert_false(jsc_value_is_typed_array(value.get())); > + g_assert_cmpuint(jsc_value_typed_array_get_type(value.get()), ==, JSC_TYPED_ARRAY_NONE); You can probably move to the next test that creates an array buffer > Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3050 > + auto value = adoptGRef(jsc_value_new_array_buffer(context.get(), point3d, sizeof(point3d), nullptr, nullptr)); > + checker.watch(value.get()); here > Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3160 > + } > +} I miss a test checking any case of exceptions.
(In reply to Carlos Garcia Campos from comment #20) > Comment on attachment 454112 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454112&action=review > > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1677 > > + case kJSTypedArrayTypeNone: > > + case kJSTypedArrayTypeArrayBuffer: return JSC_TYPED_ARRAY_NONE; > > I think this is easier to read if returns are in new lines. At first I > thought you had forgotten to add a return for the None, then I realized we > return NONE also for ArrayBuffer. Again, I think the C API is confusing, we > should either add a comment explaining what happens with the ArrayBuffer > type, or not use JSValueGetTypedArrayType and simply check > toJSTypedArrayType(object->classInfo(vm)->typedArrayStorageType) directly I think I'll change this to use the internal API directly, for the sake of clarity. In the end it will not be much longer, just a few lines more. > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1705 > > + * The @type must *not* be #JSC_TYPED_ARRAY_NONE. > > #JSC_TYPED_ARRAY_NONE -> %JSC_TYPED_ARRAY_NONE 👌️ > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1714 > > + g_return_val_if_fail(JSC_IS_CONTEXT(context), nullptr); > > + > > g_return_val_if_fail(type != JSC_TYPED_ARRAY_NONE, nullptr); 👌️ > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1742 > > +JSCValue* jsc_value_new_typed_array_with_buffer(JSCValue* arrayBuffer, JSCTypedArrayType type) > > I think we want to expose the constructor taking an offset too. The JS API > allows to pass only a buffer, buffer + offset and buffer + offset + length. > Adding 3 constructors would be too much so we can probably leave this and > add with_offset taking both offset and length, but length being optional, or > simply add offset and length here, making both optional. Let's have only jsc_value_new_typed_array_with_buffer(bufer, type, offset, length), to avoid proliferation of constructors. It's easy enough to provide zero as the offset for the default behaviour, and we can use ssize_t/gssize for the length to allow passing “-1” to mean “calculate the length from the size of the buffer”. Using a signed ssize_t/gssize for the length is fine because the JS specification says that 32-bit unsigned integers must be used, and ssize_t/gssize is 64-bit in all the architectures we support, which can always fit an unsigned 32-bit integer without loss. > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1783 > > + return type != kJSTypedArrayTypeNone && type != kJSTypedArrayTypeArrayBuffer; > > This is confusing too. Let's use here the internal C++ API as well, for clarity. > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1850 > > + auto* ptr = JSObjectGetTypedArrayBytesPtr(jsContext, jsObject, &exception); > > ptr -> bytes, data or something like that 👌️ > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1860 > > + const auto c = JSObjectGetTypedArrayLength(jsContext, jsObject, &exception); > > c -> length, size or something like that 👌️ > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1870 > > + * jsc_value_typed_array_get_count: > > I would use length instead of count 👌️ > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1899 > > + * Obtain the %ArrayBuffer for the memory region of the typed array elements. > > I guess this returns an array buffer even when the types array was not > created with a buffer, right? I would document it and add a test case if > there isn't. All typed arrays are backed by an ArrayBuffer. Typed arrays are technically a view on the contents of their buffers (and the JS spec says so) :-) > > Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3039 > > + g_assert_false(jsc_value_is_typed_array(value.get())); > > + g_assert_cmpuint(jsc_value_typed_array_get_type(value.get()), ==, JSC_TYPED_ARRAY_NONE); > > You can probably move to the next test that creates an array buffer Will do. > > Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3050 > > + auto value = adoptGRef(jsc_value_new_array_buffer(context.get(), point3d, sizeof(point3d), nullptr, nullptr)); > > + checker.watch(value.get()); > > here > > > Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3160 > > + } > > +} > > I miss a test checking any case of exceptions. Good point. Also I will add getters for the offset and byte length (as _get_size, keeping _get_length for the number of elements), as we have discussed in a chat earlier today.
Created attachment 454538 [details] Patch This patch should address all the review comments =)
Comment on attachment 454538 [details] Patch The ios-wk2 EWS failure is definitely unrelated, this patch only touches the JSC GLib API, which is not used on iOS at all :]
Comment on attachment 454538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454538&action=review > Source/JavaScriptCore/API/glib/JSCValue.cpp:1748 > + g_return_val_if_fail(JSC_IS_CONTEXT(context), nullptr); > + g_return_val_if_fail(type != JSC_TYPED_ARRAY_NONE, nullptr); > Source/JavaScriptCore/API/glib/JSCValue.cpp:1772 > + * The @type must *not* be #JSC_TYPED_ARRAY_NONE. #JSC_TYPED_ARRAY_NONE -> %JSC_TYPED_ARRAY_NONE > Source/JavaScriptCore/API/glib/JSCValue.cpp:1790 > + g_return_val_if_fail(JSC_IS_VALUE(arrayBuffer), nullptr); > + g_return_val_if_fail(jsc_value_is_array_buffer(arrayBuffer), nullptr); > + g_return_val_if_fail(type != JSC_TYPED_ARRAY_NONE, nullptr); > + We explicitly say that -1 is used for omitting length, so I would add g_return_val_if_fail(length >= -1, nullptr); > Source/JavaScriptCore/API/glib/JSCValue.cpp:1849 > + using namespace JSC; Adding JSC:: is not that much. > Source/JavaScriptCore/API/glib/JSCValue.cpp:1901 > + * ##JSCTypedArrayType), and has the `offset` over the underlying array ##JSCTypedArrayType -> #JSCTypedArrayType > Source/JavaScriptCore/API/glib/JSCValue.cpp:1934 > + auto* ptr = JSObjectGetTypedArrayBytesPtr(jsContext, jsObject, &exception); prt -> bytes
(In reply to Carlos Garcia Campos from comment #24) > Comment on attachment 454538 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454538&action=review > > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1748 > > + g_return_val_if_fail(JSC_IS_CONTEXT(context), nullptr); > > + > > g_return_val_if_fail(type != JSC_TYPED_ARRAY_NONE, nullptr); Ah, good catch. I actually wrote the prerequisite in the documentation comment but then forgot to write it in the code 😇️ > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1772 > > + * The @type must *not* be #JSC_TYPED_ARRAY_NONE. > > #JSC_TYPED_ARRAY_NONE -> %JSC_TYPED_ARRAY_NONE 👌️ > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1790 > > + g_return_val_if_fail(JSC_IS_VALUE(arrayBuffer), nullptr); > > + g_return_val_if_fail(jsc_value_is_array_buffer(arrayBuffer), nullptr); > > + g_return_val_if_fail(type != JSC_TYPED_ARRAY_NONE, nullptr); > > + > > We explicitly say that -1 is used for omitting length, so I would add > g_return_val_if_fail(length >= -1, nullptr); Good point, let's add this check in the code. > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1849 > > + using namespace JSC; > > Adding JSC:: is not that much. Sure, I'll change to use namespace-prefixed types before landing. > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1901 > > + * ##JSCTypedArrayType), and has the `offset` over the underlying array > > ##JSCTypedArrayType -> #JSCTypedArrayType 👌️ > > Source/JavaScriptCore/API/glib/JSCValue.cpp:1934 > > + auto* ptr = JSObjectGetTypedArrayBytesPtr(jsContext, jsObject, &exception); > > prt -> bytes 👌️
Created attachment 454578 [details] Patch for landing
Committed r291229 (248384@main): <https://commits.webkit.org/248384@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 454578 [details].
<rdar://problem/90248012>
This introduced a new clang warning. /app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1: warning: non-void function does not return a value in all control paths [-Wreturn-type] } ^ 1 warning generated.
(In reply to Philippe Normand from comment #29) > This introduced a new clang warning. > > /app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1: > warning: non-void function does not return a value in all control paths > [-Wreturn-type] > } > ^ > 1 warning generated. Fixed in r291543 — thanks for the heads up :)
(In reply to Adrian Perez from comment #30) > (In reply to Philippe Normand from comment #29) > > This introduced a new clang warning. > > > > /app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1: > > warning: non-void function does not return a value in all control paths > > [-Wreturn-type] > > } > > ^ > > 1 warning generated. > > Fixed in r291543 — thanks for the heads up :) Originally the patch had This is going to break compilation in GCC 8.3. (In reply to Adrian Perez from comment #30) > (In reply to Philippe Normand from comment #29) > > This introduced a new clang warning. > > > > /app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1: > > warning: non-void function does not return a value in all control paths > > [-Wreturn-type] > > } > > ^ > > 1 warning generated. > > Fixed in r291543 — thanks for the heads up :) This change broke compilation in GCC 8.3 (Debian Stable bot and Ubuntu 18.04) https://build.webkit.org/#/builders/46/builds/12338 Originally the patch was like this, but I moved RELEASE_ASSERT_NOT_REACHED(); inside "switch" precisely because it broke the Debian Stable build. See: https://github.com/WebKit/webkit/compare/9d5e85c84fe6%5E..9d5e85c84fe6
(In reply to Diego Pino from comment #31) > (In reply to Adrian Perez from comment #30) > > (In reply to Philippe Normand from comment #29) > > > This introduced a new clang warning. > > > > > > /app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1: > > > warning: non-void function does not return a value in all control paths > > > [-Wreturn-type] > > > } > > > ^ > > > 1 warning generated. > > > > Fixed in r291543 — thanks for the heads up :) > > > Originally the patch had > This is going to break compilation in GCC 8.3. > (In reply to Adrian Perez from comment #30) > > (In reply to Philippe Normand from comment #29) > > > This introduced a new clang warning. > > > > > > /app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1: > > > warning: non-void function does not return a value in all control paths > > > [-Wreturn-type] > > > } > > > ^ > > > 1 warning generated. > > > > Fixed in r291543 — thanks for the heads up :) > > This change broke compilation in GCC 8.3 (Debian Stable bot and Ubuntu 18.04) > > https://build.webkit.org/#/builders/46/builds/12338 > > Originally the patch was like this, but I moved > RELEASE_ASSERT_NOT_REACHED(); inside "switch" precisely because it broke the > Debian Stable build. See: > https://github.com/WebKit/webkit/compare/9d5e85c84fe6%5E..9d5e85c84fe6 Nevermind, fixed in https://trac.webkit.org/changeset/291551/webkit.
(In reply to Diego Pino from comment #32) > (In reply to Diego Pino from comment #31) > > (In reply to Adrian Perez from comment #30) > > > (In reply to Philippe Normand from comment #29) > > > > This introduced a new clang warning. > > > > > > > > /app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1: > > > > warning: non-void function does not return a value in all control paths > > > > [-Wreturn-type] > > > > } > > > > ^ > > > > 1 warning generated. > > > > > > Fixed in r291543 — thanks for the heads up :) > > > > > > Originally the patch had > > This is going to break compilation in GCC 8.3. > > (In reply to Adrian Perez from comment #30) > > > (In reply to Philippe Normand from comment #29) > > > > This introduced a new clang warning. > > > > > > > > /app/webkit/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:2992:1: > > > > warning: non-void function does not return a value in all control paths > > > > [-Wreturn-type] > > > > } > > > > ^ > > > > 1 warning generated. > > > > > > Fixed in r291543 — thanks for the heads up :) > > > > This change broke compilation in GCC 8.3 (Debian Stable bot and Ubuntu 18.04) > > > > https://build.webkit.org/#/builders/46/builds/12338 > > > > Originally the patch was like this, but I moved > > RELEASE_ASSERT_NOT_REACHED(); inside "switch" precisely because it broke the > > Debian Stable build. See: > > https://github.com/WebKit/webkit/compare/9d5e85c84fe6%5E..9d5e85c84fe6 > > Nevermind, fixed in https://trac.webkit.org/changeset/291551/webkit. The fix makes sense, thanks for taking care of it 🙏️