Patch for review
Nexcastellan
the following. It's for compile-time compatibility with the following
extensions, some of which already worked with my previous commits:
json, rmagick, imagescience, mechanize, hpricot, recaptcha.
http://users.nexopia.com/uploads/3233/3233577/rubinius/0001-gem-extension.patch
I'd appreciate fixes to any mistakes I made, along with brief
descriptions. Alternatively, if you just tell me some function is
broken, that's not as good but still a bit helpful.
Cezar Sá Espinola
Great work, I think there need to be some further work though. Some comments on parts that got my attention below. Someone please correct me if I'm wrong.
rb_Integer: It should try calling "to_int" first (rb_check_convert_type), "to_i" is a fallback. (Although I don't know in which bizarre world "to_i" return would be different to "to_int" return).
RFLOAT: It won't work the way it's written, this line:
ret->value = *(double *)AS_HNDL(obj)->data;
will cause a segfault because data will be NULL.
You should take a closer look at RSTRING and "check_rstruct_data" in handle.c to copy data back to subtend. RFLOAT should look something like:
RFloat RFLOAT(VALUE obj) {
RFloat * ret;
ret = (RFloat*)AS_HNDL(obj)->data;
if(!ret) {
ret = ALLOC(RFloat);
ret->value = *((double*)BYTES_OF(HNDL(obj)));
AS_HNDL(obj)->data = (void*)ret;
}
return ret;
}
And you should change check_rstruct_data adding:
else if(FLOAT_P(o)) {
RFloat *rf = (RFloat*)h->data;
*((double*)BYTES_OF(o)) = rf->value;
XFREE(rf);
}
RHASH: I didn't got into details but it looks like hash_table members aren't the same as MRI st_table, this could break stuff. Also you should follow the same R* pattern, saving the struct instance on rni_handle->data and copying back data to subtend later. The way it is now it will leak tons of memory since each call to RHASH will instantiate a new RHash that will never get freed.
One last tip, writing specs to each of the functions you're adding will give you a very good feedback on your progress. You can write the specs and run them against MRI first, when you have your specs running on MRI it's just a matter of fixing the code until it passes the specs.
I would be glad trying to help you more but I'll probably only have type to have fun with Rubinius again on the weekend, very sad.... :-(
Good luck, and keep hacking! :)
-
Cezar Sá Espinola
Christopher Thompson
> Hi Nexcastellan,
>
> Great work, I think there need to be some further work though. Some
> comments on parts that got my attention below. Someone please correct me
> if I'm wrong.
>
> rb_Integer: It should try calling "to_int" first
>
> RFLOAT: It won't work the way it's written, this line:
> ret->value = *(double *)AS_HNDL(obj)->data;
> will cause a segfault because data will be NULL.
> You should take a closer look at RSTRING and "check_rstruct_data" in
> handle.c to copy data back to subtend. RFLOAT should look something like:
> RFloat RFLOAT(VALUE obj) {
> RFloat * ret;
> ret = (RFloat*)AS_HNDL(obj)->data;
> if(!ret) {
> ret = ALLOC(RFloat);
> ret->value = *((double*)BYTES_OF(HNDL(obj)));
> AS_HNDL(obj)->data = (void*)ret;
> }
> return ret;
> }
> And you should change check_rstruct_data adding:
> else if(FLOAT_P(o)) {
> RFloat *rf = (RFloat*)h->data;
> *((double*)BYTES_OF(o)) = rf->value;
> XFREE(rf);
> }
Thank you, the key part I was missing was the check_rstruct_data.
That's why I could not understand the RHASH, RSTRING, etc. conversions.
> RHASH: I didn't got into details but it looks like hash_table members
> aren't the same as MRI st_table, this could break stuff. Also you should
> follow the same R* pattern, saving the struct instance on
> rni_handle->data and copying back data to subtend later. The way it is
> now it will leak tons of memory since each call to RHASH will
> instantiate a new RHash that will never get freed.
True, but now I have the information to fix it. Thanks. :) There's no
doubt that my implementation wasn't the same as MRI's. It would have
worked (for a limited definition of "work") for the extensions I was
testing, but I'll get a more complete implementation that should work
for other extensions.
> Good luck, and keep hacking! :)
Thank you very much for your comments. The check_rstruct_data is what
was eluding me, and this area has now snapped into clear focus. :) I
really appreciate your feedback.
Christopher Thompson
> Hi Nexcastellan,
>
> Great work, I think there need to be some further work though. Some
> comments on parts that got my attention below. Someone please correct me
> if I'm wrong.
There's a new version of this patch, for anyone who cares to look, at:
http://users.nexopia.com/uploads/3233/3233577/rubinius/RMI-extension.patch
I believe I addressed all the issues that Cezar raised. I'll most
likely commit this tomorrow. Any remaining issues, and there may well
be some, I'll most likely track down as I start debugging issues with
Rubinius running the Nexopia ruby code. I already have found some MRI
incompatibilities in the language itself and I'm quite sure the gem
extensions will also break things.
As to the hash table, I decided to copy in MRI's st.h and st.c. It
seemed to be the easiest approach.
Cezar Sá Espinola
On rb_check_type:
Instead of using rb_class2name(x), you should have rb_obj_classname(x). Also, passing a T_* constant to subtend_get_global() is currently meaningless. Take a look at rb_type(), one option is to take struct type_map out of the function. Other option is adjusting the T_* enum so that the constants match the expected value on subtend_get_global(). I thing the later is a better option as it would also be possible to clean rb_type() up.
When I get back from work I'll take a better look at the patch.
-
Cezar