Created attachment 230271 [details] [review]
arg: Throw an explicit error if we don't know how to make an array

Review of attachment 230271 [details] [review]:

::: gi/arg.c
@@ +1097,3 @@
+                  JSVAL_IS_OBJECT(value) ? JSVAL_TO_OBJECT(value) : NULL);
+        g_free(display_name);
+        return JS_FALSE;

g_base_info_unref();

(We really need to introduce caching at some point...)

::: test/js/testGIMarshalling.js
@@ +209,3 @@
+    assertRaises(function() {
+	GIMarshallingTests.array_uint8_in(bytes);
+    });

I wonder if need also a test for:

GIMarshallingTests.array_uint8_in({ length: 4, 0: 97, 1: 98, 2: 99, 3: 100 });

Created attachment 230294 [details] [review]
arg: Fix memory leak in exception path in array conversion

Created attachment 230295 [details] [review]
arg: Throw an explicit error if we don't know how to make an array

Previously, we were just passing a 0 length array if you gave say {}
as the argument for a uint8 array...kind of lame.

Created attachment 230296 [details] [review]
tests: Add test for object that looks like array

The first patch fixes pre-existing memory leaks; I've rebased the second patch to build on top of that to not leak too.

Review of attachment 230294 [details] [review]:

Sure.

Review of attachment 230295 [details] [review]:

Makes sense.

Review of attachment 230296 [details] [review]:

A comment would be appreciated saying that this testing the pattern used for certain types, and what those types are. We have special-cases for byte arrays, right?

walters> Jasper, can you say what you wanted more of in https://bugzilla.gnome.org/show_bug.cgi?id=689343#c9 ?   Basically what we're testing here is that we accept any object that implements the "array protocol" of having a "length" attribute and numeric attribute for 0..length
<walters> something like that as a comment in the test source?
<Jasper> walters, what real-world objects do we expect to go through that code path?
<Jasper> ByteArray?
<walters> all real js arrays do
<walters> just passing [1,2,3,4] to a uint8 array will
<walters> but here we're making an object that "looks like" an array
<walters> there is an explicit fastpath for passing byteArray to a uint8 array
<walters> but if say you pass a byteArray to a uint16 array, it will also hit this path
<walters> make sense?
* mhr3 has quit (Ping timeout: 600 seconds)
* stefw is now known as stefw_afk
* kerrick (~kerrick@kerrick.student.iastate.edu) has joined #introspection
<Jasper> walters, what's this test for if all JS arrays hit this path?
<walters> basically what happens when you do [1,2,3]  and {length: 3, 0: 1, 1: 2, 2:3} is semantically equivalent but different things in the JS interpreter itself
<walters> we support both
* owen (~otaylor@pool-74-104-161-15.bstnma.fios.verizon.net) has joined #introspection
<walters> even though we could call JS_IsArrayObject() to differentiate
* jrb has quit (Ping timeout: 600 seconds)
* donri has quit (Leaving)
* kerrick has quit (Read error: 104 (Connection reset by peer))
* kerrick (~kerrick@kerrick.student.iastate.edu) has joined #introspection
* owen has quit (Ping timeout: 600 seconds)
<walters> Jasper, so...right? =)
<walters> hey while we're talking about arrays
<walters> is there no js equivalent of .append() from python?
<walters> er, extend()
<Jasper> .push() --
<Jasper> still .push
<walters> hm, i guess this is http://stackoverflow.com/questions/1374126/how-to-append-an-array-to-an-existing-javascript-array
<Jasper> foo.push.apply(foo, elems)
<walters> right
<Jasper> I guess you could also do foo.splice(foo.length, 0, elems)
* walters did somearray += ['foo', 'bar'] but that really doesn't do what you expect...
* stfacc has quit (Ping timeout: 600 seconds)
<walters> anyways, do we have a conclusion on the above?
<Jasper> walters, I guess I don't see why it's necessary if we already take those same codepaths elsewhere
<walters> because in the JS interpreter they are different things
<walters> look at it this way - when we start doing typed array fastpaths, that will be another example of differentiating on array internals
<Jasper> but we don't switch on JS_IsArrayObject yet
<Jasper> yes, they're two separate things, but we iterate over them in exactly the same way
<walters> it'd actually be legal for the interpreter to give you a UInt8Array if you do [1,254,42]
<Jasper> No, that's not legal.
<walters> well i mean, one that downgrades to double if you store something outside the range
<Jasper> ES5 spec mandates that it constructs an object with prototype Array
<walters> yeah ok, sorry that's not what i meant
<Jasper> "
<Jasper>     Let array be the result of creating a new object as if by the expression new Array() where Array is the standard built-in constructor with that name."
<walters> more that there could be a more optimized variant, and we could take advantage of that
<walters> yes right, but that's JS semantics, whereas we can do more since we can look at the interpreter internals
<Jasper> Oh, I see.
<walters> \o/
<walters> persistence wins

Attachment 230294 [details] pushed as c1c274c - arg: Fix memory leak in exception path in array conversion
Attachment 230295 [details] pushed as 2312df2 - arg: Throw an explicit error if we don't know how to make an array