Created attachment 226802 [details] [review]
glib-2.0: make sure to null terminate the argument of string.joinv

Created attachment 226805 [details] [review]
glib-2.0: make sure to null terminate the argument of string.joinv

Test added.

Review of attachment 226805 [details] [review]:

If the patch is changed to keep the overhead at a minimum, I'm ok with merging it. However, we may want to consider handling this in a more general matter in the code generator to make it work for all functions expecting null-terminated (string) arrays.

::: vapi/glib-2.0.vapi
@@ +946,3 @@
+	public static string _joinv (string separator, [CCode (array_length = false, array_null_terminated = true)] string[] str_array);
+	public static string joinv (string separator, string[] str_array) {
+		string[] strv = str_array;

This makes an unconditional deep copy of the string array. I think we should avoid this in the case that the string array is already null-terminated.

Created attachment 226862 [details] [review]
glib-2.0: make sure to null terminate the argument of string.joinv

Don't copy the array if it is already null terminated.

--
Thanks.  I agree with that it would be better handled in codegen,
but attaching a fix fwiw.

Created attachment 226954 [details] [review]
glib-2.0: make sure to null terminate the argument of string.joinv

Oops, fixed typo in array access (str_array[-1] -> str_array[str_array.length - 1]).

Review of attachment 226954 [details] [review]:

+		if (str_array.length > 0 && str_array[str_array.length - 1] == null) {

Arrays created by valac are null-terminated by default.  If you create an n member array valac will actually allocate room for n+1 members and set the last one to NULL.  This is a pretty common case, so instead of checking str_array[str_array.length - 1] we should probably check str_array.length].

+			return _joinv (separator, str_array);
+		} else {
+			// make sure to null terminate str_array
+			string[] strv = str_array;
+			strv += null;
+			return _joinv (separator, strv);

This is still a lot of potential copying.  valac will allocate an array, *g_strdup each element*, resize the array (possibly having to allocate a whole new one), set the last element to NULL, pass it to g_strjoinv, then free each element in the array before finally freeing the array itself.

How about something like

public static string joinv (string separator, string[] str_array) {
  string res;
  if (str_array.length > -1 && str_array[str_array.length] != null) {
    unowned string[] data = (string[]) GLib.malloc (sizeof(string) * (str_array.length + 1));
    GLib.Memory.copy (data, str_array, sizeof(string) * str_array.length);
    GLib.Memory.set (&(data[str_array.length]), 0, sizeof(string));
    res = string._joinv (separator, data);
    GLib.free (data);
  } else {
    res = string._joinv (separator, str_array);
  }
  return res;
}

It's a bit ugly (and hackish) but it gets the job done with a single malloc/free.  The other option is to just re-implement g_strjoinv which would save us a malloc but IMHO that's probably going a bit overboard.

FWIW I like the idea of implementing this in codegen instead.

(In reply to comment #6)
> Arrays created by valac are null-terminated by default.  If you create an n
> member array valac will actually allocate room for n+1 members and set the last
> one to NULL.  This is a pretty common case, so instead of checking
> str_array[str_array.length - 1] we should probably check str_array.length].

You're right that we would need to check [length] instead of [length - 1] to detect already NULL-terminated arrays. However, if the array is not already NULL-terminated, accessing [length] might crash the process. I don't currently see a solution that works without the help of the code generator.

(In reply to comment #8)
> You're right that we would need to check [length] instead of [length - 1] to
> detect already NULL-terminated arrays. However, if the array is not already
> NULL-terminated, accessing [length] might crash the process. I don't currently
> see a solution that works without the help of the code generator.

Do you see a solution which works /with/ the help of the code generator?  We have no way of knowing whether it is safe to check [length] for arrays returned by libraries.

The best I can think of is checking each element of the array in order until we find a NULL or reach [length].  At least then we're not making a crash /more/ likely (if we do access [length] g_strjoinv would have done so anyways).

Also, what about cases where the method we're calling wants to change the array?  Something like qsort but without nmemb.

Created attachment 227123 [details] [review]
glib-2.0: port string.joinv to Vala

Thanks for the insightful comments.

So, for this particular case, how about implementing joinv entirely in Vala, as the actual length information is available in Vala level?

Review of attachment 227123 [details] [review]:

Unfortunately, Vala doesn't always know the array length.  Sometimes it will pass -1 and expect it to be treated as a null-terminated array...  IIRC anything which returns a no_array_length unowned array falls into this category.

Also, I would prefer to use g_stpcpy to avoid the second strlen.

commit 3c0674fafe5d11362fe3f88bb4c849c34789b5d5
Author: Evan Nemerson <evan@coeus-group.com>
Date:   Fri Jan 24 23:21:05 2014 -0800

    glib-2.0: make string.joinv handle non-null-terminated arrays
    
    Fixes bug 686451.

There seems to be a problem with handling empty array argument: see bug 723195.

Cool, confirmed it's working now.  Thanks.