Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rb_check_convert_type compatibility with MRI #2862

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions spec/ruby/optional/capi/object_spec.rb
Expand Up @@ -169,6 +169,13 @@ class DescObjectTest < ObjectTest
@o.rb_check_convert_type(ao).should == []
@o.rb_check_convert_type(h).should == nil
end

it "raises when the coercion method returns a different object" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always include the exception name in the spec description.

it "raises a TypeError..."

dummy = mock('to_ary')
dummy.should_receive(:to_ary).and_return(10)

lambda { @o.rb_check_convert_type(dummy) }.should raise_error(TypeError)
end
end

describe "rb_check_array_type" do
Expand Down
2 changes: 1 addition & 1 deletion vm/capi/float.cpp
Expand Up @@ -68,6 +68,6 @@ extern "C" {
}

VALUE rb_Float(VALUE object_handle) {
return rb_convert_type(object_handle, 0, "Float", "to_f");
return rb_convert_type(object_handle, -1, "Float", "to_f");
}
}
2 changes: 1 addition & 1 deletion vm/capi/numeric.cpp
Expand Up @@ -275,7 +275,7 @@ extern "C" {
return env->get_handle(ret);
}

return rb_convert_type(object_handle, 0, "Integer", "to_i");
return rb_convert_type(object_handle, -1, "Integer", "to_i");
}

void rb_num_zerodiv(void) {
Expand Down
31 changes: 26 additions & 5 deletions vm/capi/object.cpp
Expand Up @@ -118,24 +118,45 @@ extern "C" {
return rb_funcall(env->get_handle(env->state()->globals().type.get()), rb_intern("try_convert"), 3, object_handle, rb_cString, rb_intern("to_str"));
}

VALUE rb_check_convert_type(VALUE object_handle, int /*type*/,
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the following multi-line /* comment style:

/* this is a comment
 * that spans multiple lines
 */

NOTE: when `0` is given as the `type` no error will be raised. This is due to
the way this function is used in Rbx itself and Rbx not having MRI's
`convert_type` function.
*/
VALUE rb_check_convert_type(VALUE object_handle, int type,
const char* type_name, const char* method_name)
{
NativeMethodEnvironment* env = NativeMethodEnvironment::get();
VALUE name = env->get_handle(String::create(env->state(), method_name));
VALUE retval = Qnil;

if(RTEST(rb_funcall(object_handle, rb_intern("respond_to?"), 1, name)) ) {
return rb_funcall2(object_handle, rb_intern(method_name), 0, NULL);
retval = rb_funcall2(object_handle, rb_intern(method_name), 0, NULL);
}

return Qnil;
// If the method returns nil we can bail out right away.
if ( NIL_P(retval) ) return retval;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if(NIL_P(retval))


/*
When the coercion method exists but returns a different type than specified
in `type` MRI will raise an error. This code is mostly a copy-paste job
from the MRI source code.
*/
if ( type != -1 && TYPE(retval) != type ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

const char *cname = rb_obj_classname(object_handle);

rb_raise(rb_eTypeError, "can't convert %s to %s (%s#%s gives %s)",
cname, type_name, cname, method_name, rb_obj_classname(retval));
}

return retval;
}

VALUE rb_check_to_integer(VALUE object_handle, const char *method_name) {
if(FIXNUM_P(object_handle)) {
return object_handle;
}
VALUE result = rb_check_convert_type(object_handle, 0, "Integer", method_name);
VALUE result = rb_check_convert_type(object_handle, -1, "Integer", method_name);
if(rb_obj_is_kind_of(result, rb_cInteger)) {
return result;
}
Expand Down Expand Up @@ -375,7 +396,7 @@ extern "C" {
}

VALUE rb_to_int(VALUE object_handle) {
return rb_convert_type(object_handle, 0, "Integer", "to_int");
return rb_convert_type(object_handle, -1, "Integer", "to_int");
}

VALUE rb_hash(VALUE obj) {
Expand Down
4 changes: 2 additions & 2 deletions vm/capi/string.cpp
Expand Up @@ -167,7 +167,7 @@ extern "C" {
}

VALUE rb_String(VALUE object) {
return rb_convert_type(object, 0, "String", "to_s");
return rb_convert_type(object, -1, "String", "to_s");
}

void rb_str_modify(VALUE self) {
Expand Down Expand Up @@ -363,7 +363,7 @@ extern "C" {
}

VALUE rb_str_to_str(VALUE object) {
return rb_convert_type(object, 0, "String", "to_str");
return rb_convert_type(object, -1, "String", "to_str");
}

VALUE rb_string_value(volatile VALUE* object_variable) {
Expand Down