Forum: Ruby-Gnome 2 Converting array of Strings to C array of strings

This pattern is repeated a couple of times in glib2 and gtk2:

    for (i = 0; i < gargc; i++) {
        if (TYPE(RARRAY_PTR(argary)[i]) == T_STRING) {
            gargv[i+1] = RVAL2CSTR(RARRAY_PTR(argary)[i]);
        }
        else {
            gargv[i+1] = g_strdup("");
        }
    }

The TYPE() == T_STRING test seems like a poor choice.  Shouldn’t we at
least try using #to_str?

Also, as this pattern is repeated I really think that we should have a
utility function for it.  I’ll write it, but I need input on the
TYPE() comparison above.

Hi,

In <CADdV=MueLiSH3ag9mDeFgKhYdTYbpqVUWFXtLfXUj2hsbVrydw@mail.gmail.com>
  "[ruby-gnome2-devel-en] Converting array of Strings to C array of 
strings" on Fri, 9 Sep 2011 14:28:05 +0200,
  Nikolai Weibull <now@bitwi.se> wrote:

>
> The TYPE() == T_STRING test seems like a poor choice.  Shouldnt we at
> least try using #to_str?
>
> Also, as this pattern is repeated I really think that we should have a
> utility function for it.  Ill write it, but I need input on the
> TYPE() comparison above.

Use the following code that uses #to_str:

  for (i = 0; i < gargc; i++) {
      gargv[i+1] = RVAL2CSTR(RARRAY_PTR(argary)[i]);
  }


Thanks,
--
kou

2011/9/10 Kouhei Sutou <kou@cozmixng.org>:

>  Nikolai Weibull <now@bitwi.se> wrote:

>>
>> The TYPE() == T_STRING test seems like a poor choice.  Shouldn’t we at
>> least try using #to_str?

> Use the following code that uses #to_str:
>
>  for (i = 0; i < gargc; i++) {
>      gargv[i+1] = RVAL2CSTR(RARRAY_PTR(argary)[i]);
>  }

Good.  I will update rbg_rval2argv (well, remove it, I suppose).

Hi,

2011/9/10 Kouhei Sutou <kou@cozmixng.org>:
>>       gargv[i+1] = RVAL2CSTR(RARRAY_PTR(argary)[i]);
>> utility function for it. Ill write it, but I need input on the
>> TYPE() comparison above.
>
> Use the following code that uses #to_str:
>
> for (i = 0; i < gargc; i++) {
>   gargv[i+1] = RVAL2CSTR(RARRAY_PTR(argary)[i]);
> }

BTW. The code is not GC-safe.
Suppose argary[0].to_str returns a newly allocated string object S and
garbage collection happens during execution of argary[1].to_str.
In this case, it is possible that S is reclaimed by GC and gargv[0]
becomes dangling pointer.

-- Masahiro

On Sun, Oct 2, 2011 at 10:24, Masahiro Sakai <sakai@tom.sfc.keio.ac.jp> 
wrote:

>>  for (i = 0; i < gargc; i++) {
>>      gargv[i+1] = RVAL2CSTR(RARRAY_PTR(argary)[i]);
>>  }
>
> BTW. The code is not GC-safe.
> Suppose argary[0].to_str returns a newly allocated string object S and
> garbage collection happens during execution of argary[1].to_str.
> In this case, it is possible that S is reclaimed by GC and gargv[0]
> becomes dangling pointer.

How do we solve this?  This problem probably appears in a lot of places.

On Sun, Oct 2, 2011 at 11:42, Nikolai Weibull <now@bitwi.se> wrote:
>> becomes dangling pointer.
>
> How do we solve this?  This problem probably appears in a lot of places.

I can only think of having two arrays, one for VALUEs and one for C
pointers.  Does that work?  And, is there an easier way of doing it?

I'm sorry but I misunderstood.
RVAL2CSTR stores results of #to_str into RARRAY_PTR(argary)[i],
so that newly allocated objects are reachable and will not be GC'ed.

(But, of course, modifying RARRAY_PTR(argary)[i] directly is not a
good idea and causes problems in some cases. For example it modify
the array even if the array is frozen. Also it might affect other arrays
if contents of the array are shared with other arrays.)

Regards,

-- Masahiro

2011/10/2 Nikolai Weibull <now@bitwi.se>:

On Sun, Oct 2, 2011 at 14:40, Masahiro Sakai <sakai@tom.sfc.keio.ac.jp> 
wrote:
> I'm sorry but I misunderstood.
> RVAL2CSTR stores results of #to_str into RARRAY_PTR(argary)[i],
> so that newly allocated objects are reachable and will not be GC'ed.

Oh, yes, that’s true, even for arrays.

> (But, of course, modifying RARRAY_PTR(argary)[i] directly is not a
> good idea and causes problems in some cases. For example it modify
> the array even if the array is frozen. Also it might affect other arrays
> if contents of the array are shared with other arrays.)

Yes, this is probably even worse.  Hm, I guess copying the Ruby array
and then using RVAL2CSTR on the copy is the best thing to do.  I’ll
look into creating a patch.  (I have some other things queued up
affecting this code anyway.)

On Sun, Oct 2, 2011 at 18:48, Nikolai Weibull <now@bitwi.se> wrote:
>> if contents of the array are shared with other arrays.)
>
> Yes, this is probably even worse.  Hm, I guess copying the Ruby array
> and then using RVAL2CSTR on the copy is the best thing to do.  I’ll
> look into creating a patch.  (I have some other things queued up
> affecting this code anyway.)

OK, so the new interface will be

const gchar **rbg_rval2strv(volatile VALUE *ary, long *n);
#define RVAL2STRS(ary, n) rbg_rval2strv(&(ary), &(n))

Is that fine with everyone?  I would have prefered

#define RVAL2STRS(ary, n) rbg_rval2strv(ary, n)

simply because it’s more explicit, but just passing in the variable
meshes with RVAL2STR/StringValue/StringValuePtr.

What do you think?

Once we agree on an interface I will apply all the necessary changes.

(The same interface will then apply to all RVAL2*(ary, n) 
macros/functions.)

Note that the functionality will change so that

*ary = rb_ary_dup(rb_ary_to_ary(*ary));

will be performed before modifying the values or ary, thus making sure
that we don’t change either ary or the result of #to_ary.

On Sat, Oct 8, 2011 at 16:19, Nikolai Weibull <now@bitwi.se> wrote:

> OK, so the new interface will be
>
> const gchar **rbg_rval2strv(volatile VALUE *ary, long *n);
> #define RVAL2STRS(ary, n) rbg_rval2strv(&(ary), &(n))
>
> Is that fine with everyone?

This has now been implemented.