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

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

Thanks for reviewing!

Hi,

In [email protected]ain.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());
  1. Please don’t use ‘//’ style comment. Please use ‘/* */’ instead.
  2. Please define all variable at the begin of block.
  3. Please fill your code in 80 columns.
  4. 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

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?

  1. 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. - http://zarb.org/~gc/

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.

  1. 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

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.

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

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

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,

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. - http://zarb.org/~gc/

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

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

This forum is not affiliated to the Ruby language, Ruby on Rails framework, nor any Ruby applications discussed here.

| Privacy Policy | Terms of Service | Remote Ruby Jobs