-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
[Truffle] Add check_convert_type and check_funcall methods #4146
Conversation
Just saw this today, maybe it can help: |
public void setRunning(boolean running) { | ||
this.running = running; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is state
in CoreLibrary for this 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've updated this to use state
Could you explain the semantics as you understand them? |
@@ -398,7 +398,8 @@ private void createAccesor(DynamicObject module, String name) { | |||
final RubyNode sequence = Translator.sequence(getContext(), sourceSection.getSource(), rubySourceSection, Arrays.asList(checkArity, accessInstanceVariable)); | |||
final RubyRootNode rootNode = new RubyRootNode(getContext(), sourceSection, null, sharedMethodInfo, sequence, false); | |||
final CallTarget callTarget = Truffle.getRuntime().createCallTarget(rootNode); | |||
final InternalMethod method = new InternalMethod(sharedMethodInfo, accessorName, module, visibility, callTarget); | |||
final boolean builtIn = !getContext().isRunning(); | |||
final InternalMethod method = new InternalMethod(sharedMethodInfo, accessorName, module, visibility, builtIn, callTarget); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic could be part of addMethod or InternalMethod constructor to avoid duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved this to the constructor
Just a thought: maybe check_funcall can be implemented with sth like |
@eregon |
I don't believe |
I see.
Right, the Rubinius code seemed pretty specific but MRI isn't. |
Let's look at the C then: VALUE
rb_check_funcall(VALUE recv, ID mid, int argc, const VALUE *argv)
{
return rb_check_funcall_default(recv, mid, argc, argv, Qundef);
}
VALUE
rb_check_funcall_default(VALUE recv, ID mid, int argc, const VALUE *argv, VALUE def)
{
VALUE klass = CLASS_OF(recv);
const rb_callable_method_entry_t *me;
rb_thread_t *th = GET_THREAD();
int respond = check_funcall_respond_to(th, klass, recv, mid);
if (!respond) // if check_funcall_respond_to returns 0, return Qundef
return def;
me = rb_search_method_entry(recv, mid);
if (!check_funcall_callable(th, me)) {
return check_funcall_missing(th, klass, recv, mid, argc, argv,
respond, def);
}
stack_check(th);
return vm_call0(th, recv, mid, argc, argv, me);
}
static int
check_funcall_respond_to(rb_thread_t *th, VALUE klass, VALUE recv, ID mid)
{
return vm_respond_to(th, klass, recv, mid, TRUE);
}
static int
vm_respond_to(rb_thread_t *th, VALUE klass, VALUE obj, ID id, int priv)
{
VALUE defined_class;
const ID resid = idRespond_to;
// This is looking up respond_to? on the metaclass of the object
const rb_method_entry_t *const me =
method_entry_get(klass, resid, &defined_class);
if (!me) return -1; // if no respond_to?, assume it's OK
if (METHOD_ENTRY_BASIC(me)) {
// if respond_to? is built-in (maybe we could just check if it is Kernel#respond_to?
// since that is the only built-in respond_to?), return -1 which means "OK keep going"
return -1;
}
else {
// Some arity checking, doesnt seem important for now
int argc = 1;
VALUE args[2];
VALUE result;
args[0] = ID2SYM(id);
args[1] = Qtrue;
if (priv) {
argc = rb_method_entry_arity(me);
if (argc > 2) {
rb_raise(rb_eArgError,
"respond_to? must accept 1 or 2 arguments (requires %d)",
argc);
}
if (argc != 1) {
argc = 2;
}
else if (!NIL_P(ruby_verbose)) {
VALUE location = rb_method_entry_location(me);
rb_warn("%"PRIsVALUE"%c""respond_to?(:%"PRIsVALUE") uses"
" the deprecated method signature, which takes one parameter",
(FL_TEST(klass, FL_SINGLETON) ? obj : klass),
(FL_TEST(klass, FL_SINGLETON) ? '.' : '#'),
QUOTE_ID(id));
if (!NIL_P(location)) {
VALUE path = RARRAY_AREF(location, 0);
VALUE line = RARRAY_AREF(location, 1);
if (!NIL_P(path)) {
rb_compile_warn(RSTRING_PTR(path), NUM2INT(line),
"respond_to? is defined here");
}
}
}
}
// if obj's respond_to? is not Kernel's and is defined, actually call it
// aka !!meth.call(name)
result = call_method_entry(th, defined_class, obj, resid, me, argc, args);
return RTEST(result);
}
} |
def self.convert_type(obj, cls, meth, raise) | ||
r = check_funcall(obj, meth) | ||
if undefined.equal?(r) | ||
if raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't name it raise
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the name of this param
@@ -82,6 +82,16 @@ def self.object_respond_to?(obj, name, include_private = false) | |||
Truffle.invoke_primitive :vm_object_respond_to, obj, name, include_private | |||
end | |||
|
|||
def self.object_respond_to_no_built_in?(obj, name, include_private = false) | |||
meth = Truffle.invoke_primitive :vm_method_lookup, obj, name | |||
!meth.nil? && !(Truffle.invoke_primitive :vm_method_is_basic, meth) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move parenthesis around the call here: !Truffle.invoke_primitive(:vm_method_is_basic, meth)
r | ||
end | ||
|
||
def self.check_funcall(recv, meth, args = undefined) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
args should probably default to []
or be removed if it's never used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've update this to []
and used them later on to send *args
|
||
def self.check_funcall_missing(recv, meth, args, respond, default) | ||
ret = basic_obj_respond_to_missing(recv, meth, false) #PRIV false | ||
return default unless undefined.equal?(ret) || ret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is equivalent I think:
if (!RTEST(ret)) return def;
But this might be clearer:
res = basic_obj_respond_to_missing(recv, meth, false) #PRIV false
return default unless res
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this to your recommendation, thanks
return recv.__send__(:method_missing, meth) # , args | ||
rescue NoMethodError | ||
# TODO BJF usually more is done here | ||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the missing logic here is causing the failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eregon I think you right, I've added a new commit with more check_funcall_missing
implemented which resolves the spec failures
begin | ||
return recv.__send__(:method_missing, meth, *args) | ||
rescue NoMethodError | ||
# TODO BJF usually more is done here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that TODO addressed now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not completely, I think there's some cases that still aren't handled here yet: https://github.com/ruby/ruby/blob/dba03c38c54835248766f28033b5aeddc5f052ef/vm_eval.c#L369
Looks good, congrats for translating this fairly complex code! |
@eregon Thanks for the help with this. I forgot to mention, this was the last untagged core array spec, so array is at 100% now, 🎉 |
This is a work in progress. Please review.
This is for issue #3992.
I'm currently having difficulty here with 3 examples regressing. Here is a short example:
Output:
Expected: