[Truffle] Adding Array#shuffle to array.rb.

[bjfish]

No description provided.

[nirvdrum]

This shouldn't need to be changed as @tuple.swap should work fine. We handle that in BodyTranslator.

[bjfish]

@nirvdrum regarding your comments, here is some discussion from jruby channel, does this make sense?

bjfish2: chrisseaton when implementing the array.rb methods should I continue pulling in more tuple.rb methods to make these work? https://github.com/rubinius/rubinius/blob/v2.4.1/kernel/bootstrap/array.rb
[08:43am] chrisseaton: tuple is a Rubinius primitive we don't implement - we just use an array - so you might find it doesn't always work
[08:43am] chrisseaton: which method were you thinking of that uses tuple?
[08:44am] bjfish2: shuffle, sample
[08:44am] bjfish2: https://github.com/rubinius/rubinius/blob/v2.4.1/kernel/common/array.rb
[08:44am] bjfish2: chrisseaton i think there’s more
[08:45am] chrisseaton: bjfish2: in this case I think @tuple is just self (or it needs to be if it isn't), so that might work
[08:46am] chrisseaton: bjfish2: see visitInstVarNode in BodyTranslator to see where @tuple will come from
[08:47am] bjfish2: chrisseaton it’s currently implemented as self, but if I want to call methods defined in tuple.rb i need ‘@tuple’ to return self.to_tuple
[08:49am] chrisseaton: bjfish2: the methods in Tuple are all very low level - I'm not sure it's worth trying to bring it all in - could we just shim the methods it has directly in Array?
[08:58am] bjfish2: chrisseaton yes the methods could go directly in array, they should then probably look something like this, does this look right? https://gist.github.com/bjfish/f247a80ffd5b0fa2306a
[08:59am] chrisseaton: You don't need to do tuple.at(a), you can simply do at(a), as tuple is self
[08:59am] chrisseaton: Can you put a big comment that these methods came from tuple.rb?
[09:01am] bjfish2: yep, got it, i don’t think the method needs to be singleton either if @tuple is just self

[nirvdrum]

This makes sense. We've just been avoiding modifications to the Rubinius source where we can. Since I'm coming into this one late, I'll let @chrisseaton review.

[nirvdrum]

And not to overly complicate things, but I had a similar issue with @data in String. I worked around it with some composition & delegation. I wrote my own data structure (Rubinius::StringData to implement @data):

jruby/truffle/src/main/ruby/core/shims.rb

Lines 204 to 226 in 5a506d2

	module Rubinius
	class StringData
	attr_accessor :array
	def initialize(array)
	@array = array
	end
	def equal?(other)
	@array == other.array
	end
	alias_method :==, :equal?
	def size
	@array.size
	end
	def [](index)
	@array[index]
	end
	end
	end

and passed the String bytes in BodyTranslator:

jruby/truffle/src/main/java/org/jruby/truffle/translator/BodyTranslator.java

Lines 1593 to 1600 in 5a506d2

	} else if (nameWithoutSigil.equals("@data")) {
	final RubyNode bytes = new RubyCallNode(context, sourceSection, "bytes",
	new SelfNode(context, sourceSection), null, false);
	return new RubyCallNode(context, sourceSection, "new",
	new ObjectLiteralNode(context, sourceSection, context.getCoreLibrary().getStringDataClass()),
	null, false, bytes);
	}

That's not to say what I did was right or better, but it's another option. Our notion of @tuple just needs to be API-compatible with how Rubinius calls it.

[chrisseaton]

truffle/shims/tuple.rb is actually probably a better place for these methods, as @nirvdrum suggests. That keeps them separate from the real Array methods. You'll need to put the Rubinius license header into that file.

The extra wrapper might work well for String, but I think things are generally a little simpler here. The methods on Tuple just look like a subset of the methods on Array, so we might as well just use it as an array.

But whatever works for you @bjfish and is clean is good. It's better to try out some different ideas at this stage than it is to work too hard to make everything totally consistent.

Can you move to shims and then I'll merge?

[bjfish]

@chrisseaton I think I would @tuple would need to return a kind_of? Tuple then to use instance methods from shims/tuple.rb

@nirvdrum would you be able to help in modifying body translator so that @tuple returns a Tuple.class

[chrisseaton]

The method can still be a member of Array, just inside the class shims/tuple.rb. I know that sounds a bit strange, but it means if you were looking for the Rubinius Tuple methods they'd be in tuple.rb.

[bjfish]

@nirvdrum let me know what you think of the update, I still left the shuffle method modified to use swap( , ) instead of @tuple.swap( , ) because I thought swap should be a private array method

[nirvdrum]

My preference is to use @tuple.swap to make future syncs with Rubinius easier. Being a private method, while better than being public, still exposes a method in Array that shouldn't be there -- that's something we're going to have to eventually deal with anyway.

[bjfish]

@nirvdrum Okay, I've updated this to make swap public and use @tuple.swap

[nirvdrum]

Looks good. If it turns out I'm a moron, we can move this back easily enough :-)