I have always, as have many of you, hated the fact that Block.yield
accepts a RubyArray. There are reasons for this, though they're not
particularly good..
When yielding values to a block, if you yield an array directly it will
be implicitly splatted into the target arguments. It's somewhat
inconsistent with what you'd expect, and I believe it's changing in Ruby
2.0:
def foo; yield 1,2; end
def bar; yield [1, 2]; end
foo {|a, b| p a; p b; }
bar {|a, b| p a; p b; }
# both print out 1, then 2, rather than [1, 2] and nil in the second
case as you might expect.
Because arrays can be unwrapped in this way, the entire yield call path
has always accepted IRubyObject. So things became complicated:
yield 1 # resulted in 1 getting stuffed into a RubyArray
yield 1, 2 # resulted in [1, 2] being yielded
yield [1, 2] # same
yield [[1, 2]] # yielded [[1, 2]], treating [1, 2] as a single param
And there's a bunch of weird splat behavior I won't go into here because
I don't fully understand it well enough to explain.
The problem with all this is that we had to wrap basically all arguments
to blocks with RubyArray instances, since that's the only way they'll
pass through the arg assignment processing. That's ugly overhead. It
also massively complicates the compiler, since it needs to do all that
same ugly logic.
Attached is a patch that alters yield to always accept IRubyObject[],
like a normal call, and changes all callers and implementations to
handle that fact. There are ups and downs to this:
up: we don't have to wrap with RubyArray just to pass values
up: it's simpler to follow
down: sometimes it wraps with an extraneous IRubyObject[]
down: the assignment logic is not yet converted over
This is part of my work to get the remainder of block arg types
compiling. I also have prototype code locally for multiple assignment
which is very close to complete.
Please try out the patch if you can. If you understand block yield
semantics (yeah right) let me know if I'm handling this stuff correctly.
ant test passes and gems install.
- Charlie
Index: src/org/jruby/runtime/CallBlock.java
===================================================================
--- src/org/jruby/runtime/CallBlock.java (revision 3918)
+++ src/org/jruby/runtime/CallBlock.java (working copy)
@@ -63,7 +63,7 @@
}
public IRubyObject yield(ThreadContext context, IRubyObject value) {
- return yield(context, value, null, null, false);
+ return yield(context, new IRubyObject[] {value}, null, null, false);
}
/**
@@ -76,7 +76,7 @@
* @param aValue Should value be arrayified or not?
* @return
*/
- public IRubyObject yield(ThreadContext context, IRubyObject value,
IRubyObject self,
+ public IRubyObject yield(ThreadContext context, IRubyObject[] args,
IRubyObject self,
RubyModule klass, boolean aValue) {
if (klass == null) {
self = this.self;
@@ -86,7 +86,6 @@
pre(context, klass);
try {
- IRubyObject[] args = new IRubyObject[] {value};
// This while loop is for restarting the block call in case a
'redo' fires.
while (true) {
try {
Index: src/org/jruby/runtime/MethodBlock.java
===================================================================
--- src/org/jruby/runtime/MethodBlock.java (revision 3918)
+++ src/org/jruby/runtime/MethodBlock.java (working copy)
@@ -31,6 +31,7 @@
***** END LICENSE BLOCK *****/
package org.jruby.runtime;
+import org.jruby.RubyArray;
import org.jruby.RubyMethod;
import org.jruby.RubyModule;
import org.jruby.RubyProc;
@@ -103,7 +104,7 @@
}
public IRubyObject call(ThreadContext context, IRubyObject[] args) {
- return yield(context, context.getRuntime().newArrayNoCopy(args), null,
null, true);
+ return yield(context, args, null, null, true);
}
protected void pre(ThreadContext context, RubyModule klass) {
@@ -115,7 +116,7 @@
}
public IRubyObject yield(ThreadContext context, IRubyObject value) {
- return yield(context, value, null, null, false);
+ return yield(context, new IRubyObject[] {value}, null, null, false);
}
/**
@@ -128,7 +129,7 @@
* @param aValue Should value be arrayified or not?
* @return
*/
- public IRubyObject yield(ThreadContext context, IRubyObject value,
IRubyObject self,
+ public IRubyObject yield(ThreadContext context, IRubyObject[] args,
IRubyObject self,
RubyModule klass, boolean aValue) {
if (klass == null) {
self = this.self;
@@ -141,7 +142,7 @@
// This while loop is for restarting the block call in case a
'redo' fires.
while (true) {
try {
- return callback.execute(value, new IRubyObject[] { method,
self }, NULL_BLOCK);
+ return
callback.execute(RubyArray.newArrayNoCopyLight(context.getRuntime(), args), new
IRubyObject[] { method, self }, NULL_BLOCK);
} catch (JumpException je) {
if (je.getJumpType() == JumpException.JumpType.RedoJump) {
context.pollThreadEvents();
Index: src/org/jruby/runtime/CompiledBlock.java
===================================================================
--- src/org/jruby/runtime/CompiledBlock.java (revision 3918)
+++ src/org/jruby/runtime/CompiledBlock.java (working copy)
@@ -66,22 +66,22 @@
}
public IRubyObject call(ThreadContext context, IRubyObject[] args) {
- return yield(context, context.getRuntime().newArrayNoCopy(args), null,
null, true);
+ return yield(context, args, null, null, true);
}
- public IRubyObject yield(ThreadContext context, IRubyObject args,
IRubyObject self, RubyModule klass, boolean aValue) {
+ public IRubyObject yield(ThreadContext context, IRubyObject[] args,
IRubyObject self, RubyModule klass, boolean aValue) {
if (klass == null) {
self = this.self;
frame.setSelf(self);
}
IRubyObject[] realArgs = null;
- if (!aValue) {
- // handle as though it's just an array coming in...i.e. it should
be multiassigned or just assigned as is to var 0.
- // FIXME for now, since masgn isn't supported, this just wraps
args in an IRubyObject[], since single vars will want that anyway
- realArgs = new IRubyObject[] {args};
- } else {
- realArgs = ArgsUtil.convertToJavaArray(args);
- }
+// if (aValue) {
+// realArgs = args;
+// } else {
+// realArgs = ArgsUtil.convertToJavaArray(args[0]);
+// }
+ // FIXME: Assuming it's already set up correctly when it gets here?
+ realArgs = args;
try {
pre(context, klass);
return callback.call(context, self, realArgs);
Index: src/org/jruby/runtime/SharedScopeBlock.java
===================================================================
--- src/org/jruby/runtime/SharedScopeBlock.java (revision 3918)
+++ src/org/jruby/runtime/SharedScopeBlock.java (working copy)
@@ -60,7 +60,7 @@
}
public IRubyObject call(ThreadContext context, IRubyObject[] args,
IRubyObject replacementSelf) {
- return yield(context, context.getRuntime().newArrayNoCopy(args), null,
null, true);
+ return yield(context, args, null, null, true);
}
public Block cloneBlock() {
Index: src/org/jruby/runtime/Block.java
===================================================================
--- src/org/jruby/runtime/Block.java (revision 3918)
+++ src/org/jruby/runtime/Block.java (working copy)
@@ -59,9 +59,10 @@
return false;
}
- public IRubyObject yield(ThreadContext context, IRubyObject value,
IRubyObject self,
+ public IRubyObject yield(ThreadContext context, IRubyObject[] args,
IRubyObject self,
RubyModule klass, boolean aValue) {
- throw context.getRuntime().newLocalJumpError("noreason",
(IRubyObject)value, "yield called out of block");
+ // FIXME: with args as IRubyObject[], no obvious second param
here...
+ throw context.getRuntime().newLocalJumpError("noreason", self,
"yield called out of block");
}
public Block cloneBlock() {
@@ -171,7 +172,7 @@
}
public IRubyObject call(ThreadContext context, IRubyObject[] args) {
- return yield(context, context.getRuntime().newArrayNoCopy(args), null,
null, true);
+ return yield(context, args, null, null, true);
}
protected void pre(ThreadContext context, RubyModule klass) {
@@ -183,7 +184,7 @@
}
public IRubyObject yield(ThreadContext context, IRubyObject value) {
- return yield(context, value, null, null, false);
+ return yield(context, new IRubyObject[] {value}, null, null, false);
}
/**
@@ -196,8 +197,8 @@
* @param aValue Should value be arrayified or not?
* @return
*/
- public IRubyObject yield(ThreadContext context, IRubyObject value,
IRubyObject self,
- RubyModule klass, boolean aValue) {
+ public IRubyObject yield(ThreadContext context, IRubyObject[] args,
IRubyObject self,
+ RubyModule klass, boolean useArrayWithoutConversion) {
if (klass == null) {
self = this.self;
frame.setSelf(self);
@@ -207,10 +208,10 @@
try {
if (iterNode.getVarNode() != null) {
- if (aValue) {
- setupBlockArgs(context, iterNode.getVarNode(), value,
self);
+ if (useArrayWithoutConversion) {
+ setupBlockArgs(context, iterNode.getVarNode(),
RubyArray.newArrayNoCopyLight(context.getRuntime(), args), self);
} else {
- setupBlockArg(context, iterNode.getVarNode(), value, self);
+ setupBlockArg(context, iterNode.getVarNode(), args[0],
self);
}
}
Index: src/org/jruby/evaluator/EvaluationState.java
===================================================================
--- src/org/jruby/evaluator/EvaluationState.java (revision 3918)
+++ src/org/jruby/evaluator/EvaluationState.java (working copy)
@@ -1828,7 +1828,12 @@
Block block = context.getCurrentFrame().getBlock();
- return block.yield(context, result, null, null,
iVisited.getCheckState());
+ if (iVisited.getCheckState()) {
+ // unwrap rubyarray as direct arguments for passing to yield
+ return block.yield(context, ((RubyArray)result).toJavaArray(),
null, null, true);
+ }
+
+ return block.yield(context, new IRubyObject[] {result}, null, null,
iVisited.getCheckState());
}
private static IRubyObject zArrayNode(Ruby runtime) {
Index: src/org/jruby/RubyObject.java
===================================================================
--- src/org/jruby/RubyObject.java (revision 3918)
+++ src/org/jruby/RubyObject.java (working copy)
@@ -839,16 +839,13 @@
block.setVisibility(Visibility.PUBLIC);
try {
- IRubyObject valueInYield;
boolean aValue;
if (args.length == 1) {
- valueInYield = args[0];
aValue = false;
} else {
- valueInYield = RubyArray.newArray(getRuntime(), args);
aValue = true;
}
- return block.yield(context, valueInYield, selfInYield,
context.getRubyClass(), aValue);
+ return block.yield(context, args, selfInYield,
context.getRubyClass(), aValue);
//TODO: Should next and return also catch here?
} catch (JumpException je) {
if (je.getJumpType() ==
JumpException.JumpType.BreakJump) {
Index: src/org/jruby/RubyArray.java
===================================================================
--- src/org/jruby/RubyArray.java (revision 3918)
+++ src/org/jruby/RubyArray.java (working copy)
@@ -2194,7 +2194,7 @@
ThreadContext context = getRuntime().getCurrentContext();
IRubyObject obj1 = (IRubyObject) o1;
IRubyObject obj2 = (IRubyObject) o2;
- IRubyObject ret = block.yield(context, getRuntime().newArray(obj1,
obj2), null, null, true);
+ IRubyObject ret = block.yield(context, new IRubyObject[] {obj1,
obj2}, null, null, true);
int n = RubyComparable.cmpint(ret, obj1, obj2);
//TODO: ary_sort_check should be done here
return n;
Index: src/org/jruby/RubyHash.java
===================================================================
--- src/org/jruby/RubyHash.java (revision 3918)
+++ src/org/jruby/RubyHash.java (working copy)
@@ -945,7 +945,7 @@
for (int i = 0; i < ltable.length; i++) {
for (RubyHashEntry entry = ltable[i]; entry != null && (entry
= checkIter(ltable, entry)) != null; entry = entry.next) {
// rb_assoc_new equivalent
- block.yield(context, RubyArray.newArray(runtime,
entry.key, entry.value), null, null, false);
+ block.yield(context, new IRubyObject[]
{RubyArray.newArray(runtime, entry.key, entry.value)}, null, null, false);
}
}
} finally {postIter();}
@@ -966,7 +966,7 @@
for (int i = 0; i < ltable.length; i++) {
for (RubyHashEntry entry = ltable[i]; entry != null && (entry
= checkIter(ltable, entry)) != null; entry = entry.next) {
// rb_yield_values(2,...) equivalent
- block.yield(context, RubyArray.newArray(runtime,
entry.key, entry.value), null, null, true);
+ block.yield(context, new IRubyObject[] {entry.key,
entry.value}, null, null, true);
}
}
} finally {postIter();}
@@ -1210,7 +1210,7 @@
RubyHashEntry[]ltable = table;
for (int i = 0; i < ltable.length; i++) {
for (RubyHashEntry entry = ltable[i]; entry != null && (entry
= checkIter(ltable, entry)) != null; entry = entry.next) {
- if (block.yield(context, RubyArray.newArray(runtime,
entry.key, entry.value), null, null, true).isTrue())
+ if (block.yield(context, new IRubyObject[] {entry.key,
entry.value}, null, null, true).isTrue())
delete(entry.key, block);
}
}
Index: src/org/jruby/compiler/Compiler.java
===================================================================
--- src/org/jruby/compiler/Compiler.java (revision 3920)
+++ src/org/jruby/compiler/Compiler.java (working copy)
@@ -122,7 +122,7 @@
* If arguments have been prepared for the block, specify true. Otherwise
the default
* empty args will be used.
*/
- public void yield(boolean hasArgs);
+ public void yield(boolean hasArgs, boolean unwrap);
/**
* Assigns the value on top of the stack to a local variable at the
specified index, consuming
Index: src/org/jruby/compiler/impl/StandardASMCompiler.java
===================================================================
--- src/org/jruby/compiler/impl/StandardASMCompiler.java (revision 3920)
+++ src/org/jruby/compiler/impl/StandardASMCompiler.java (working copy)
@@ -495,7 +495,7 @@
}
}
- public void yield(boolean hasArgs) {
+ public void yield(boolean hasArgs, boolean unwrap) {
loadClosure();
SkinnyMethodAdapter method = getMethodAdapter();
@@ -514,11 +514,18 @@
method.aconst_null();
}
+ if (unwrap) {
+ method.checkcast(cg.p(RubyArray.class));
+ method.invokevirtual(cg.p(RubyArray.class), "toJavaArray",
cg.sig(IRubyObject[].class));
+ } else {
+ createObjectArray(1);
+ }
+
method.aconst_null();
method.aconst_null();
- method.ldc(Boolean.FALSE);
+ method.ldc(new Boolean(unwrap));
- method.invokevirtual(cg.p(Block.class), "yield",
cg.sig(IRubyObject.class, cg.params(ThreadContext.class, IRubyObject.class,
IRubyObject.class, RubyModule.class, Boolean.TYPE)));
+ method.invokevirtual(cg.p(Block.class), "yield",
cg.sig(IRubyObject.class, cg.params(ThreadContext.class, IRubyObject[].class,
IRubyObject.class, RubyModule.class, Boolean.TYPE)));
}
private void invokeIRubyObject(String methodName, String signature) {
Index: src/org/jruby/compiler/YieldNodeCompiler.java
===================================================================
--- src/org/jruby/compiler/YieldNodeCompiler.java (revision 3918)
+++ src/org/jruby/compiler/YieldNodeCompiler.java (working copy)
@@ -31,6 +31,6 @@
NodeCompilerFactory.getCompiler(yieldNode.getArgsNode()).compile(yieldNode.getArgsNode(),
context);
}
- context.yield(yieldNode.getArgsNode() != null);
+ context.yield(yieldNode.getArgsNode() != null,
yieldNode.getCheckState());
}
}
---------------------------------------------------------------------
To unsubscribe from this list please visit:
http://xircles.codehaus.org/manage_email
