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

[Truffle] Allow Rubinius primitives to be written entirely in Ruby #3076

Merged
merged 11 commits into from
Jun 25, 2015

Conversation

chrisseaton
Copy link
Contributor

@eregon @nirvdrum @pitr-ch please review and sign off when you are done.

@chrisseaton chrisseaton added this to the truffle-dev milestone Jun 22, 2015
@@ -31,6 +30,7 @@ def self.module_mirror(obj)
end
end
end
Truffle::Primitive.install_rubinius_primitive self, :module_mirror
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to get self in a @CoreMethod anymore - onSingleton and isModuleFunction both set needsSelf to false.

Copy link
Member

Choose a reason for hiding this comment

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

In this particular case, it's intended to not be obvious, since it would be likely wrong with a module function (defined on both instance and class level) as self could be either a module or the surrounding self.
If it is just a singleton/class method, then it might be right to want it and currently there is constuctor for this (which is literally onSingleton + needsSelf), which was appropriately named when I looked at current use-cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I thought that self was the module when it isn't. I think I will use some magic to get the self of the caller, since this slow path stuff.

@chrisseaton chrisseaton self-assigned this Jun 22, 2015
new LiteralNode(context, sourceSection, callConstructor.getModule()),
null,
false,
arguments.toArray(new RubyNode[arguments.size()]));
Copy link
Member

Choose a reason for hiding this comment

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

Can we share this with above so the code is not duplicated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only the arguments array that can be shared. I didn't do it originally as the body of the loop is a bit different, but I can probably do a second pass to handle that.

@eregon
Copy link
Member

eregon commented Jun 22, 2015

Sounds like a nice addition.
Maybe we could take a (bound) Method as argument and passing the primitive name?
And additionally call the method directly (or its CallTarget) so even if it is overwritten the behavior stays identical.
This would allow to do something like

Truffle::Primitive.install_rubinius_primitive :vm_object_class, Kernel.method(:class)

Which would solve the currently not very useful primitives just delegating to something else and possible monkey-patching issues.

@chrisseaton
Copy link
Contributor Author

Yes that's a good idea.

The main motivation for this was to start writing some primitives using Java interop. vm_time and vm_gc are both early candidates for this as they just do a single Java static call.

@nirvdrum
Copy link
Contributor

This looks good. However, I am concerned about the ability to find these primitive methods. While having a node that serves only to delegate to a RubyCallNode seems superfluous at face value, it does have the benefit of being consistent with primitives written in Java. I.e., they can all be found in an expected location.

If we want to go this route, I propose introducing a new directory for implemented mirror methods. api/shims may have made sense for that one case at that time, but I think could stand from better organization now. This would still require searches in two places, but at least they're two well-known locations.

I would maybe even take it a step further and have the translator look at the source section to ensure Truffle::Primitive.install_rubinius_primitive can only be used from this directory, throwing an error otherwise. That might be a bit heavy-handed (particularly for a lint tool), but it would help keep us honest.

@chrisseaton
Copy link
Contributor Author

A new directory for mirror methods? Do you mean a directory for primitive methods?

@nirvdrum
Copy link
Contributor

Sorry, yes, for primitives. The mirror stuff I think could be tidied up a bit too, but that's a separate issue.

@eregon
Copy link
Member

eregon commented Jun 22, 2015

I agree, but maybe a file is enough? It should be loaded just before alpha.rb I think.

@nirvdrum
Copy link
Contributor

A single file is fine with me. I just don't want that to become unwieldly. Ruby is much more concise than Java for this though, so it may not be a warranted concern.

@eregon
Copy link
Member

eregon commented Jun 25, 2015

I'll do the change to make it take a Ruby Method.

@eregon eregon force-pushed the truffle-ruby-primitives branch from 4be9644 to 364d745 Compare June 25, 2015 09:39
* Receiver + method name in one argument.
@pitr-ch
Copy link
Member

pitr-ch commented Jun 25, 2015

👍 looks good to me

eregon added a commit that referenced this pull request Jun 25, 2015
[Truffle] Allow Rubinius primitives to be written entirely in Ruby
@eregon eregon merged commit a0d0a1d into master Jun 25, 2015
@eregon
Copy link
Member

eregon commented Jun 25, 2015

I merged with a few changes to accept (bound) Method.

I tried as well with UnboundMethod, but it does not fit for vm_ primitives since their self is ignored, while the corresponding Kernel methods use self.

@eregon eregon deleted the truffle-ruby-primitives branch June 25, 2015 16:48
@enebo enebo added this to the Non-Release milestone Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants