[Truffle] Trying to add #to_ary to Array#replace #2689
Conversation
| @Specialization | ||
| public Object coerceObject(VirtualFrame frame, Object object) { | ||
| notDesignedForCompilation(); | ||
| if (object instanceof RubyArray) { |
There was a problem hiding this comment.
This should be its own specialization so in the common case of just being an array, you get a very small method body that could be inlined. You'll then need to guard the generic case.
There was a problem hiding this comment.
To clarify, one specialisation should be RubyArray, and then other should be Object and then also guarded !isRubyArray. Otherwise you could use the Object one first and then keep happily using it when you get a RubyArray as the type fits.
|
CreateCast is fine here. It's allowing you to normalize one argument of all specializations to a fixed type. I'm not sure how frequently ToAryNode will be used otherwise, but that's fine. |
| public abstract class ToAryNode extends RubyNode { | ||
|
|
||
| @Child | ||
| private CallDispatchHeadNode toAryNode; |
There was a problem hiding this comment.
Minor style thing, but in every other case I've seen we have the annotation and the field declaration on the same line.
|
Your |
bjfish
force-pushed
the
truffle_array_replace_to_ary
branch
from
af1b7c8
to
fd32663
Compare
|
@nirvdrum Thanks for all the feedback. I've updated the PR with a rebased commit that I believe addresses your comments. |
| throw e; | ||
| } | ||
| } | ||
| return coerced; |
There was a problem hiding this comment.
If you look at the other coercion nodes, they check the value of the return type. I think you need to do that here as well, since someone could define to_ary to return something other than a RubyArray. Here are the two cases I saw with Array#replace:
$ ruby -e 'class X; end; [].replace(X.new)'
-e:1:in `replace': no implicit conversion of X into Array (TypeError)
from -e:1:in `<main>'
That's the missing to_ary implementation, which you've addressed in the code above. But there's also:
$ ruby -e 'class X; def to_ary; :hello; end; end; [].replace(X.new)'
-e:1:in `replace': can't convert X to Array (X#to_ary gives Symbol) (TypeError)
from -e:1:in `<main>'
That's the case you're missing.
|
@bjfish No problem. If you can address my last comment about handling the case when |
bjfish
force-pushed
the
truffle_array_replace_to_ary
branch
from
fd32663
to
2b1f79b
Compare
|
@nirvdrum I've added to return type check, updated the return type to RubyArray (ToStrNode returns RubyString) , and fixed the guard spacing. |
| } | ||
|
|
||
| @Specialization | ||
| public Object coerceRubyArray(RubyArray rubyArray) { |
There was a problem hiding this comment.
Good point on the return type. This should be RubyArray, too.
bjfish
force-pushed
the
truffle_array_replace_to_ary
branch
from
2b1f79b
to
5444ab7
Compare
|
@nirvdrum I fixed the other return type. |
|
Looks great. |
[Truffle] Trying to add #to_ary to Array#replace
@nirvdrum @chrisseaton Is this correct place to use a CreateCast? If not, why not?
Also trying to implement ToAryNode here.