RVAL2CSTR encoding support [was: String encoding problem with ruby 1.9.2p0]

blueprint · September 27, 2010, 5:27pm

Ok, here’s my current patch for progressing on RVAL2CSTR encoding
support.

Thanks for reviewing!

blueprint · September 28, 2010, 3:16pm

Hi,

In AANLkTiktv1bw09RgUbDSL9vm0MKQo31R5oKFq=removed_email_address@domain.invalid
“[ruby-gnome2-devel-en] RVAL2CSTR encoding support [was: String
encoding problem with ruby 1.9.2p0]” on Mon, 27 Sep 2010 17:26:28 +0200,
Guillaume C. [email protected] wrote:

Ok, here’s my current patch for progressing on RVAL2CSTR encoding support.

Thanks!
It seems almost OK!

I have some comments. Could you commit after applying those
comments?

Index: glib2/ext/glib2/rbglib.c

— glib2/ext/glib2/rbglib.c (revision 3902)
+++ glib2/ext/glib2/rbglib.c (working copy)
@@ -43,6 +43,19 @@
extern void Init_glib_keyfile();
extern void Init_glib_bookmark_file();

+gchar *
+rbg_rval2cstr(VALUE str)
+{
+#ifdef HAVE_RB_STR_ENCODE
if (NIL_P(str))
 StringValue(str);  // properly trigger exception - can't convert nil into String (TypeError)
VALUE utf8_str = rb_enc_get(str) == rb_utf8_encoding() ? str : rb_str_export_to_enc(str, rb_utf8_encoding());

Please don’t use ‘//’ style comment. Please use ‘/* */’ instead.
Please define all variable at the begin of block.
Please fill your code in 80 columns.
Please use return value of StringValue(str). It
converts non-String but String like object to String.

Revised code:
VALUE utf8_str;
str = StringValue(str);
if (rb_enc_get(str) == rb_utf8_encoding()) {
utf8_str = str;
} else {
utf8_str = rb_str_export_to_enc(str, rb_utf8_encoding());
}

gchar *
rbg_rval2cstr_accept_nil(VALUE str)
{

return NIL_P(str) ? NULL : StringValuePtr(str);

return NIL_P(str) ? NULL : rbg_rval2cstr(str);
}

Please use RVAL2CSTR() macro.

Index: glib2/ext/glib2/rbglib.h

— glib2/ext/glib2/rbglib.h (revision 3902)
+++ glib2/ext/glib2/rbglib.h (working copy)

@@ -76,6 +76,7 @@
extern const gchar *rbg_rval_inspect(VALUE object);

extern gchar* rbg_string_value_ptr(volatile VALUE* ptr); /* no longer used */
+extern gchar rbg_rval2cstr(VALUE str);
extern gchar rbg_rval2cstr_accept_nil(VALUE str);
extern VALUE rbg_cstr2rval(const char str);
extern VALUE rbg_cstr2rval_with_free(gchar str);

Please also update glib2.def.

Thanks,

kou

blueprint · September 28, 2010, 3:54pm

I have some comments. Could you commit after applying those
comments?

I can, but then we will need to change many occurrences of
“rb_str_new*” to CSTR2RVAL* in a separate revision?

Please use return value of StringValue(str). It
converts non-String but String like object to String.

StringValue(str) is not for using its return value, it’s just to force
Ruby to convert the VALUE into a String and raise a TypeError
exception, as current CSTR2RVAL does. This “style” for doing it (just
writing “StringValue(str)” and dropping the result) is used all over
the code of rg2, for example many times in
“trunk/glib/ext/glib2/rbglib_convert.c”.

(I have to do this, because rc_enc_get(str) gives a segmentation fault
when the str argument is nil.)

So, we can use the return value as in your patch, but it’s not really
necessary?

Thanks

–
Guillaume C. - Guillaume Cottenceau

blueprint · September 28, 2010, 4:45pm

Hi,

In [email protected]
“Re: [ruby-gnome2-devel-en] RVAL2CSTR encoding support” on Tue, 28 Sep
2010 15:54:28 +0200,
Guillaume C. [email protected] wrote:

I have some comments. Could you commit after applying those
comments?

I can, but then we will need to change many occurrences of
“rb_str_new*” to CSTR2RVAL* in a separate revision?

Yes. Please separate those changes. We like small commits
rather than a big commit.

Please use return value of StringValue(str). It
converts non-String but String like object to String.

StringValue(str) is not for using its return value, it’s just to force
Ruby to convert the VALUE into a String and raise a TypeError
exception, as current CSTR2RVAL does. This “style” for doing it (just
writing “StringValue(str)” and dropping the result) is used all over
the code of rg2, for example many times in
“trunk/glib/ext/glib2/rbglib_convert.c”.

Ah. It’s my mistake. StringValue(str) is a destructive
operation.

str = StringValue(str);

equals to

StringValue(str);

But please always call StringValue(str). You don’t need to
call NIL_P(str).

Before:
if (NIL_P(str))
StringValue(str);

After:

StringValue(str);

If str is not String but it responds to “to_str”,
StringValue(str) converts str to String by str.to_str.

So, we can use the return value as in your patch, but it’s not really necessary?

Please ignore my change in the previous mail. Please use the
above change.

Thanks,

kou

blueprint · September 28, 2010, 5:59pm

Please ignore my change in the previous mail. Please use the
above change.

Ok. Please review my latest change. I have optimized the rbg_rval2cstr
function after your latest suggestion.

blueprint · September 29, 2010, 6:06am

Hi,

In [email protected]
“Re: [ruby-gnome2-devel-en] RVAL2CSTR encoding support” on Tue, 28 Sep
2010 17:58:34 +0200,
Guillaume C. [email protected] wrote:

Ok. Please review my latest change. I have optimized the rbg_rval2cstr
function after your latest suggestion.

Thanks!

+{
+#ifdef HAVE_RB_STR_ENCODE
StringValue(str);

if (rb_enc_get(str) != rb_utf8_encoding()) {
   str = rb_str_export_to_enc(str, rb_utf8_encoding());
}
+#endif

return StringValuePtr(str);
+}

We doesn’t need to call both StringValue and
StringValuePtr. What about this?

StringValue(str);

#ifdef HAVE_RB_STR_ENCODE
if (rb_enc_get(str) != rb_utf8_encoding()) {
str = rb_str_export_to_enc(str, rb_utf8_encoding());
}
#endif
return RSTRING_PTR(str);

Others seem to be OK to me.

Thanks,

kou

blueprint · October 11, 2010, 2:04pm

Guillaume C. [email protected] wrote:

}
+#endif

return StringValuePtr(str);
+}

I don’t think this will work reliably. As far as I understand
StringValue is a macro which directly alters the its variable argument
to point to a new VALUE. In this case we are making a temporary variable
called ‘str’ which is scoped to the lifetime of the rbg_rval2cstr
function. The new string gets stored in this variable and then we return
the char* pointer from it. However once the function returns this
VALUE is no longer pointed to by anything so it could get garbage
collected at any point.

I think the function would have to be changed so that it can modify its
argument, like this:

gchar *
rbg_rval2cstr(VALUE *str)
{
#ifdef HAVE_RB_STR_ENCODE
StringValue(*str);
if (rb_enc_get(*str) != rb_utf8_encoding()) {
*str = rb_str_export_to_enc(*str, rb_utf8_encoding());
}
#endif
return StringValuePtr(*str);
}

Then the RVAL2CSTR macro would have to take a pointer to its argument,
like so:

#define RVAL2CSTR(v) (rbg_rval2cstr(&v))

That is similar to what the StringValuePtr macro does.

Neil

blueprint · October 12, 2010, 3:33am

Hi,

Neil, you’re right.

Guillaume, could you fix it?

Thanks,

kou

In [email protected]
“Re: [ruby-gnome2-devel-en] RVAL2CSTR encoding support” on Mon, 11 Oct
2010 13:03:58 +0100,

blueprint · October 12, 2010, 9:09am

On Tue, Oct 12, 2010 at 3:32 AM, Kouhei S. [email protected] wrote:

Hi,

Neil, you’re right.

Guillaume, could you fix it?

I know too little in Ruby GC stuff in C. I suggest either that someone
gives me a documentation pointer about it and then I can try, else if
Neil or you would like to write the patch…

Thanks,

–
Guillaume C. - Guillaume Cottenceau

blueprint · October 23, 2010, 7:27am

Hi,

In [email protected]
“Re: [ruby-gnome2-devel-en] RVAL2CSTR encoding support” on Wed, 13 Oct
2010 16:20:43 +0900 (JST),
Kouhei S. [email protected] wrote:

I know too little in Ruby GC stuff in C. I suggest either that someone
gives me a documentation pointer about it and then I can try, else if
Neil or you would like to write the patch…

Umm… I don’t know GC related document in English… And I
can’t describe it in English.
(keyword: Ruby uses conservative GC.)

Neil, could you fix the problem what you described?

I’ve done it!

Thanks,

kou

blueprint · October 13, 2010, 9:21am

Hi,

In [email protected]
“Re: [ruby-gnome2-devel-en] RVAL2CSTR encoding support” on Tue, 12 Oct
2010 09:09:17 +0200,
Guillaume C. [email protected] wrote:

I know too little in Ruby GC stuff in C. I suggest either that someone
gives me a documentation pointer about it and then I can try, else if
Neil or you would like to write the patch…

Umm… I don’t know GC related document in English… And I
can’t describe it in English.
(keyword: Ruby uses conservative GC.)

Neil, could you fix the problem what you described?

Thanks,

kou