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] Correcting Array#inspect spec with inspect fixes #3997

Merged
merged 6 commits into from
Jul 7, 2016

Conversation

bjfish
Copy link
Contributor

@bjfish bjfish commented Jul 6, 2016

This Array#inspect spec was failing due to issues with the current String#inspect and lack of rb_inspect and rb_str_escape. I had to get creative/experimental to resolve all these issues.

Any issues with the following?
Are there any issue with using Truffle::CExt nodes like this in our ruby core code?
Are there any issues using the RubyString implementations?

@@ -419,6 +419,22 @@ def self.compatible_encoding(a, b)
enc
end

# similar to rb_inspect
def self.object_inspect(val)
Copy link
Member

Choose a reason for hiding this comment

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

Rubinius::Type.object_* are usually for the original version of these methods on Object/Kernel, like class, etc.
Maybe just inspect here is fine? (so we just remove the rb_ prefix)

@eregon
Copy link
Member

eregon commented Jul 6, 2016

Are there any issue with using Truffle::CExt nodes like this in our ruby core code?

Technically, it's OK since it's just a Java @CoreMethod.
But it is the wrong location.
What about making it a primitive, say string_escape in StringNodes?

It would also avoid the wrapping/unwrapping which seems wrong (rb_str_escape returns a VALUE not a char* which is what CExtString represents and no method should deal with that except the existing conversion methods).
Also CExtNodes is for helpers of the C-ext API for functionality not existing otherwise. rb_str_escape is not part of the C-ext API.

Are there any issues using the RubyString implementations?

It's not ideal because it means it's less easy to optimize and might throw jruby exceptions.
In this case it seems OK though.
In the long term, maybe this could make it into StringSupport so there is no need to build the RubyString.
(And in general a more functional-style method which does not rely on global state is always better as keeping both global states in sync is very error-prone).

@bjfish
Copy link
Contributor Author

bjfish commented Jul 6, 2016

@eregon I've refactored per your suggestions, thanks!

@@ -3103,6 +3123,19 @@ protected static boolean hasRawBytes(DynamicObject string) {

}

@CoreMethod(names = "string_escape")
Copy link
Member

Choose a reason for hiding this comment

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

Make this a primitive, like the node just below.
You can then invoke it with Truffle.invoke_primitive :string_escape, str.
This avoids polluting String's namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eregon Made it a primitive in the latest commit

Copy link
Member

Choose a reason for hiding this comment

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

Why not using invoke_primitive? I'll push a commit to do that.

@bjfish bjfish closed this Jul 7, 2016
@bjfish bjfish reopened this Jul 7, 2016
@eregon
Copy link
Member

eregon commented Jul 7, 2016

cc @jruby/truffle
If return is in almost last position (def method; ...; return :foo if condition; :bar; end), I usually prefer a if/else as return needs to throw an exception in interpreter and it also makes the flow look more functional. If it's an early exit, return is the good choice.
Also performance-wise, invoke_primitive is a nice way to insert a node directly into the tree, without call overhead and ensuring it cannot be monkey-patched. But it's quite verbose and less idiomatic so I would only use it to call semi-hidden stuff like that string_escape.

@eregon eregon added the truffle label Jul 7, 2016
@eregon eregon added this to the truffle-dev milestone Jul 7, 2016
@eregon eregon self-assigned this Jul 7, 2016
@eregon eregon merged commit d577cff into master Jul 7, 2016
@eregon eregon deleted the truffle-array-inspect branch July 7, 2016 11:36
@enebo enebo added this to the Invalid or Duplicate 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

3 participants