GNOME Bugzilla – Bug 772790
Add support for boolean, gunichar, and 64-bit int arrays
Last modified: 2016-10-20 04:57:04 UTC
The new code style guidelines codified in AX_COMPILER_FLAGS suggest compiling GNOME code with -Wswitch-enum and -Wswitch-default. Doing so revealed a couple of array types that we don't support but which would be relatively easy to add. Here are patches adding support for bool arrays, gunichar arrays (which are marshalled into strings), and [u]int64 arrays. Appropriate regression tests are added to gobject-introspection. (This is part of a longer effort to get GJS to compile warning-free with AX_COMPILER_FLAGS.)
Created attachment 337495 [details] [review] tests: Marshalling tests for new GJS functionality GJS has gained some support for marshalling arrays of previously unsupported types. These are the marshalling tests that are needed to test that new functionality.
Created attachment 337496 [details] [review] arg: Free float and double arrays Compiling with -Wswitch revealed that these are not freed correctly. They can be freed just like the other arrays of primitive types.
Created attachment 337497 [details] [review] arg: Support boolean-valued arrays Compiling with -Wswitch revealed that we don't support these, but it's actually quite trivial to do. However, we don't support zero-terminated boolean arrays as they are nonsensical, they could only contain TRUE. Throw an exception in the unlikely case that we encounter this.
Created attachment 337498 [details] [review] arg: Support 64-bit int C arrays Support for this was previously missing, as revealed by compiling with -Wswitch. Supporting it is fairly trivial, though we do have to modify gjs_array_to_intarray() since it previously handled only up to 32-bit arguments.
Created attachment 337499 [details] [review] arg: Support gunichar-valued arrays Compiling with -Wswitch revealed that we do not support gunichar arrays. This adds support for marshalling JS strings into arrays of gunichar, containing UCS-4 encoded strings, and vice versa. For JS-to-C, one can additionally supply a JS array of integers which will be marshalled into a gunichar array.
Review of attachment 337495 [details] [review]: Looks good ::: tests/gimarshallingtests.c @@ +1415,3 @@ +{ + unsigned ix; + static gunichar expected[] = GI_MARSHALLING_TESTS_CONSTANT_UCS4; Here and elsewhere, static const?
Review of attachment 337496 [details] [review]: Ok
Review of attachment 337497 [details] [review]: ::: gi/arg.cpp @@ +899,3 @@ + case GI_TYPE_TAG_BOOLEAN: + return gjs_array_to_intarray(context, array_value, length, arr_p, + sizeof(gboolean), UNSIGNED); This will convert anything that is a number to the corresponding number value. Most importantly: * 2 will be converted to (gboolean)2, which will trip many programs if by mistake they write == TRUE * 0.4 will be converted to 0 instead of 1 (while !!0.4 should be true) ::: installed-tests/js/testGIMarshalling.js @@ +75,3 @@ + assertFalse(array[1]); + assertTrue(array[2]); + assertTrue(array[3]); assertTrue/assertFalse are too lenient. I would assertEquals(array[0], true) assertEquals(array[1], false) etc. to make sure we get the JS types right. @@ +119,3 @@ GIMarshallingTests.array_in_guint64_len(array); GIMarshallingTests.array_in_guint8_len(array); + GIMarshallingTests.array_bool_in(array); Why is this passing? We're supposed to pass [true, false, true, true] not [-1, 0, 1, 2]... @@ +144,3 @@ GIMarshallingTests.garray_int_none_in([-1, 0, 1, 2]); GIMarshallingTests.garray_utf8_none_in(["0", "1", "2"]); + GIMarshallingTests.garray_bool_none_in([-1, 0, 1, 2]); Same here.
Review of attachment 337498 [details] [review]: Looks good
Review of attachment 337499 [details] [review]: Ok ::: gjs/jsapi-util-string.cpp @@ +242,3 @@ + if (len_p != NULL) + *len_p = (size_t) length; + } You could use JS_GetStringCharsAndLength and go utf16_to_ucs4 already. But then you would need to change it again for moz38 (because it's obsolete in 33) @@ +283,3 @@ + JS::RootedString str(cx, JS_NewUCString(cx, u16_string, u16_string_length)); + + if (str) Why this check? Can this be false-y?
Comment on attachment 337495 [details] [review] tests: Marshalling tests for new GJS functionality OK, pushed the gobject-introspection patch with the addition of const. Attachment 337495 [details] pushed as 8bbd0c3 - tests: Marshalling tests for new GJS functionality
Attachment 337496 [details] pushed as 54f8c27 - arg: Free float and double arrays Attachment 337498 [details] pushed as ce2b82e - arg: Support 64-bit int C arrays
Created attachment 337686 [details] [review] arg: Support gunichar-valued arrays Compiling with -Wswitch revealed that we do not support gunichar arrays. This adds support for marshalling JS strings into arrays of gunichar, containing UCS-4 encoded strings, and vice versa. For JS-to-C, one can additionally supply a JS array of integers which will be marshalled into a gunichar array.
(In reply to Giovanni Campagna from comment #10) > Review of attachment 337499 [details] [review] [review]: > > Ok > > ::: gjs/jsapi-util-string.cpp > @@ +242,3 @@ > + if (len_p != NULL) > + *len_p = (size_t) length; > + } > > You could use JS_GetStringCharsAndLength and go utf16_to_ucs4 already. > But then you would need to change it again for moz38 (because it's obsolete > in 33) Fair enough, done. It's at least more similar to the mozjs38 API than the previous attempt was. I've amended the patch. > @@ +283,3 @@ > + JS::RootedString str(cx, JS_NewUCString(cx, u16_string, > u16_string_length)); > + > + if (str) > > Why this check? Can this be false-y? Yes - JS::RootedString decays to the wrapped JSString*, so the truth value is false if the JSString* is NULL. Haven't worked on the bool patch yet, but: (In reply to Giovanni Campagna from comment #8) > Review of attachment 337497 [details] [review] [review]: > @@ +119,3 @@ > GIMarshallingTests.array_in_guint64_len(array); > GIMarshallingTests.array_in_guint8_len(array); > + GIMarshallingTests.array_bool_in(array); > > Why is this passing? We're supposed to pass [true, false, true, true] not > [-1, 0, 1, 2]... Those should be converted to booleans, which would give the expected bool array...
(In reply to Philip Chimento from comment #14) > (In reply to Giovanni Campagna from comment #10) > > > @@ +283,3 @@ > > + JS::RootedString str(cx, JS_NewUCString(cx, u16_string, > > u16_string_length)); > > + > > + if (str) > > > > Why this check? Can this be false-y? > > Yes - JS::RootedString decays to the wrapped JSString*, so the truth value > is false if the JSString* is NULL. Yeah but can the JSString* be NULL? > Haven't worked on the bool patch yet, but: > > (In reply to Giovanni Campagna from comment #8) > > Review of attachment 337497 [details] [review] [review] [review]: > > @@ +119,3 @@ > > GIMarshallingTests.array_in_guint64_len(array); > > GIMarshallingTests.array_in_guint8_len(array); > > + GIMarshallingTests.array_bool_in(array); > > > > Why is this passing? We're supposed to pass [true, false, true, true] not > > [-1, 0, 1, 2]... > > Those should be converted to booleans, which would give the expected bool > array... Yeah, I suspected g_assert_true/g_assert_false is more lenient than it should be.
Created attachment 337746 [details] [review] tests: const specifier for transfer-none out array Since we return a static const array as the out value, the function must have a const type for its out parameter.
Created attachment 337747 [details] [review] tests: Check against TRUE and FALSE for booleans For boolean arrays, we want to make sure that we actually got an array of TRUE and FALSE (1 and 0) instead of an array of integers that may have other values besides 1 and 0.
Created attachment 337748 [details] [review] arg: Support boolean-valued arrays Compiling with -Wswitch revealed that we don't support these, but it's actually quite trivial to do. However, we don't support zero-terminated boolean arrays as they are nonsensical, they could only contain TRUE. Throw an exception in the unlikely case that we encounter this.
(In reply to Giovanni Campagna from comment #15) > (In reply to Philip Chimento from comment #14) > > (In reply to Giovanni Campagna from comment #10) > > > > > @@ +283,3 @@ > > > + JS::RootedString str(cx, JS_NewUCString(cx, u16_string, > > > u16_string_length)); > > > + > > > + if (str) > > > > > > Why this check? Can this be false-y? > > > > Yes - JS::RootedString decays to the wrapped JSString*, so the truth value > > is false if the JSString* is NULL. > > Yeah but can the JSString* be NULL? Yes, JS_NewUCString can return NULL according to the documentation: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_NewString > > > Haven't worked on the bool patch yet, but: > > > > (In reply to Giovanni Campagna from comment #8) > > > Review of attachment 337497 [details] [review] [review] [review] [review]: > > > @@ +119,3 @@ > > > GIMarshallingTests.array_in_guint64_len(array); > > > GIMarshallingTests.array_in_guint8_len(array); > > > + GIMarshallingTests.array_bool_in(array); > > > > > > Why is this passing? We're supposed to pass [true, false, true, true] not > > > [-1, 0, 1, 2]... > > > > Those should be converted to booleans, which would give the expected bool > > array... > > Yeah, I suspected g_assert_true/g_assert_false is more lenient than it > should be. Even when not using g_assert_{true,false}, the int array still gets converted to booleans when marshalling into C. It's because of the !! in gjs_value_from_g_argument().
Created attachment 337954 [details] [review] tests: Remove unused variable Left over from code review fix.
Review of attachment 337954 [details] [review]: Sure
Review of attachment 337746 [details] [review]: Looks good
Review of attachment 337747 [details] [review]: Looks good
Review of attachment 337748 [details] [review]: Looks good to me.
Review of attachment 337686 [details] [review]: ::: gjs/jsapi-util-string.cpp @@ +237,3 @@ + + const jschar *utf16 = JS_GetStringCharsAndLength(cx, str, &utf16_len); +{ Should you not throw an error here? @@ -211,0 +211,68 @@ + * gjs_string_to_ucs4: + * @cx: a #JSContext + * @value: JS::Value containing a string ... 65 more ... Indentation seems to be a bit off here -- maybe it's just the review tool though :) @@ +282,3 @@ + &u16_string_length, &error); + if (!u16_string) { + error->message); This should probably say "UCS-4 string to UTF-16" @@ +295,3 @@ + value_p.setString(str); + + } Do we not need to throw an error when str is NULL?
Attachment 337746 [details] pushed as 85c2592 - tests: const specifier for transfer-none out array Attachment 337747 [details] pushed as 9111b8f - tests: Check against TRUE and FALSE for booleans Attachment 337954 [details] pushed as d782e50 - tests: Remove unused variable
Comment on attachment 337748 [details] [review] arg: Support boolean-valued arrays Attachment 337748 [details] pushed as a0f22fb - arg: Support boolean-valued arrays
Created attachment 337994 [details] [review] arg: Support gunichar-valued arrays Compiling with -Wswitch revealed that we do not support gunichar arrays. This adds support for marshalling JS strings into arrays of gunichar, containing UCS-4 encoded strings, and vice versa. For JS-to-C, one can additionally supply a JS array of integers which will be marshalled into a gunichar array.
(In reply to Cosimo Cecchi from comment #25) > Review of attachment 337686 [details] [review] [review]: > > ::: gjs/jsapi-util-string.cpp > @@ +237,3 @@ > + > + const jschar *utf16 = JS_GetStringCharsAndLength(cx, str, &utf16_len); > +{ > > Should you not throw an error here? The documentation is unclear about whether an exception will be pending when either JS_GetStringCharsAndLength() or JS_NewUCString() return NULL. I guess to make sure, we should throw an error in both those instances that you pointed out.
Review of attachment 337994 [details] [review]: Feel free to commit with this change. ::: gjs/jsapi-util-string.cpp @@ +279,3 @@ + /* intentionally using n_bytes even though glib api suggests n_chars; with + * n_chars (from g_utf8_strlen()) the result appears truncated + long length; This comment does not seem to apply?
Attachment 337994 [details] pushed as 96f8a01 - arg: Support gunichar-valued arrays