-
-
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] Allow Rubinius primitives to be written entirely in Ruby #3076
Conversation
@@ -31,6 +30,7 @@ def self.module_mirror(obj) | |||
end | |||
end | |||
end | |||
Truffle::Primitive.install_rubinius_primitive self, :module_mirror |
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'm not sure how to get self
in a @CoreMethod
anymore - onSingleton
and isModuleFunction
both set needsSelf
to false.
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.
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.
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.
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.
new LiteralNode(context, sourceSection, callConstructor.getModule()), | ||
null, | ||
false, | ||
arguments.toArray(new RubyNode[arguments.size()])); |
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.
Can we share this with above so the code is not duplicated?
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.
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.
Sounds like a nice addition. 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. |
Yes that's a good idea. The main motivation for this was to start writing some primitives using Java interop. |
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. 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. |
A new directory for mirror methods? Do you mean a directory for primitive methods? |
Sorry, yes, for primitives. The mirror stuff I think could be tidied up a bit too, but that's a separate issue. |
I agree, but maybe a file is enough? It should be loaded just before alpha.rb I think. |
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. |
I'll do the change to make it take a Ruby Method. |
…th Truffle::Primitive.install_rubinius_primitive
4be9644
to
364d745
Compare
* Receiver + method name in one argument.
👍 looks good to me |
* Since we can add primitives at runtime.
* Extract logic from BodyTranslator into RubiniusPrimitiveConstructor implementations.
[Truffle] Allow Rubinius primitives to be written entirely in Ruby
I merged with a few changes to accept (bound) Method. I tried as well with UnboundMethod, but it does not fit for |
@eregon @nirvdrum @pitr-ch please review and sign off when you are done.