Merge pull request #338 from Shopify/rb_proc_call

Use rb_proc_call* rather than rb_funcall

Author: byroot

ext/java/org/msgpack/jruby/ExtensionRegistry.java

@@ -54,8 +54,8 @@ public IRubyObject toInternalUnpackerRegistry(ThreadContext ctx) {
     return hash;
   }
 
-  public void put(RubyModule mod, int typeId, boolean recursive, IRubyObject packerProc, IRubyObject packerArg, IRubyObject unpackerProc, IRubyObject unpackerArg) {
-    ExtensionEntry entry = new ExtensionEntry(mod, typeId, recursive, packerProc, packerArg, unpackerProc, unpackerArg);
+  public void put(RubyModule mod, int typeId, boolean recursive, IRubyObject packerProc, IRubyObject unpackerProc) {
+    ExtensionEntry entry = new ExtensionEntry(mod, typeId, recursive, packerProc, unpackerProc);
     extensionsByModule.put(mod, entry);
     extensionsByTypeId[typeId + 128] = entry;
     extensionsByAncestor.clear();
@@ -114,18 +114,14 @@ public static class ExtensionEntry {
     private final int typeId;
     private final boolean recursive;
     private final IRubyObject packerProc;
-    private final IRubyObject packerArg;
     private final IRubyObject unpackerProc;
-    private final IRubyObject unpackerArg;
 
-    public ExtensionEntry(RubyModule mod, int typeId, boolean recursive, IRubyObject packerProc, IRubyObject packerArg, IRubyObject unpackerProc, IRubyObject unpackerArg) {
+    public ExtensionEntry(RubyModule mod, int typeId, boolean recursive, IRubyObject packerProc, IRubyObject unpackerProc) {
       this.mod = mod;
       this.typeId = typeId;
       this.recursive = recursive;
       this.packerProc = packerProc;
-      this.packerArg = packerArg;
       this.unpackerProc = unpackerProc;
-      this.unpackerArg = unpackerArg;
     }
 
     public RubyModule getExtensionModule() {
@@ -157,11 +153,11 @@ public IRubyObject getUnpackerProc() {
     }
 
     public RubyArray<?> toPackerTuple(ThreadContext ctx) {
-      return ctx.runtime.newArray(new IRubyObject[] {ctx.runtime.newFixnum(typeId), packerProc, packerArg});
+      return ctx.runtime.newArray(new IRubyObject[] {ctx.runtime.newFixnum(typeId), packerProc});
     }
 
     public RubyArray<?> toUnpackerTuple(ThreadContext ctx) {
-      return ctx.runtime.newArray(new IRubyObject[] {mod, unpackerProc, unpackerArg});
+      return ctx.runtime.newArray(new IRubyObject[] {mod, unpackerProc});
     }
 
     public IRubyObject[] toPackerProcTypeIdPair(ThreadContext ctx) {

ext/java/org/msgpack/jruby/Factory.java

@@ -80,43 +80,15 @@ public IRubyObject registeredTypesInternal(ThreadContext ctx) {
     });
   }
 
-  @JRubyMethod(name = "register_type", required = 2, optional = 1)
-  public IRubyObject registerType(ThreadContext ctx, IRubyObject[] args) {
+  @JRubyMethod(name = "register_type_internal", required = 3, visibility = PRIVATE)
+  public IRubyObject registerTypeInternal(ThreadContext ctx, IRubyObject type, IRubyObject mod, IRubyObject opts) {
     testFrozen("MessagePack::Factory");
 
     Ruby runtime = ctx.runtime;
-    IRubyObject type = args[0];
-    IRubyObject mod = args[1];
-
-    IRubyObject packerArg;
-    IRubyObject unpackerArg;
-
-    RubyHash options = null;
-
-    if (args.length == 2) {
-      packerArg = runtime.newSymbol("to_msgpack_ext");
-      unpackerArg = runtime.newSymbol("from_msgpack_ext");
-    } else if (args.length == 3) {
-      if (args[args.length - 1] instanceof RubyHash) {
-        options = (RubyHash) args[args.length - 1];
-        packerArg = options.fastARef(runtime.newSymbol("packer"));
-        if (packerArg != null && packerArg.isNil()) {
-          packerArg = null;
-        }
-        unpackerArg = options.fastARef(runtime.newSymbol("unpacker"));
-        if (unpackerArg != null && unpackerArg.isNil()) {
-          unpackerArg = null;
-        }
-        IRubyObject optimizedSymbolsParsingArg = options.fastARef(runtime.newSymbol("optimized_symbols_parsing"));
-        if (optimizedSymbolsParsingArg != null && optimizedSymbolsParsingArg.isTrue()) {
-          throw runtime.newArgumentError("JRuby implementation does not support the optimized_symbols_parsing option");
-        }
-      } else {
-        throw runtime.newArgumentError(String.format("expected Hash but found %s.", args[args.length - 1].getType().getName()));
-      }
-    } else {
-      throw runtime.newArgumentError(String.format("wrong number of arguments (%d for 2..3)", 2 + args.length));
-    }
+    RubyHash options = (RubyHash) opts;
+
+    IRubyObject packerProc = options.fastARef(runtime.newSymbol("packer"));
+    IRubyObject unpackerProc = options.fastARef(runtime.newSymbol("unpacker"));
 
     long typeId = ((RubyFixnum) type).getLongValue();
     if (typeId < -128 || typeId > 127) {
@@ -128,21 +100,6 @@ public IRubyObject registerType(ThreadContext ctx, IRubyObject[] args) {
     }
     RubyModule extModule = (RubyModule) mod;
 
-    IRubyObject packerProc = runtime.getNil();
-    IRubyObject unpackerProc = runtime.getNil();
-    if (packerArg != null) {
-      packerProc = packerArg.callMethod(ctx, "to_proc");
-    }
-    if (unpackerArg != null) {
-      if (unpackerArg instanceof RubyString || unpackerArg instanceof RubySymbol) {
-        unpackerProc = extModule.method(unpackerArg.callMethod(ctx, "to_sym"));
-      } else if (unpackerArg instanceof RubyProc || unpackerArg instanceof RubyMethod) {
-        unpackerProc = unpackerArg;
-      } else {
-        unpackerProc = unpackerArg.callMethod(ctx, "method", runtime.newSymbol("call"));
-      }
-    }
-
     boolean recursive = false;
     if (options != null) {
       IRubyObject recursiveExtensionArg = options.fastARef(runtime.newSymbol("recursive"));
@@ -151,7 +108,7 @@ public IRubyObject registerType(ThreadContext ctx, IRubyObject[] args) {
       }
     }
 
-    extensionRegistry.put(extModule, (int) typeId, recursive, packerProc, packerArg, unpackerProc, unpackerArg);
+    extensionRegistry.put(extModule, (int) typeId, recursive, packerProc, unpackerProc);
 
     if (extModule == runtime.getSymbol() && !packerProc.isNil()) {
       hasSymbolExtType = true;

ext/java/org/msgpack/jruby/Packer.java

@@ -93,28 +93,11 @@ public IRubyObject registeredTypesInternal(ThreadContext ctx) {
     return registry.toInternalPackerRegistry(ctx);
   }
 
-  @JRubyMethod(name = "register_type", required = 2, optional = 1)
-  public IRubyObject registerType(ThreadContext ctx, IRubyObject[] args, final Block block) {
+  @JRubyMethod(name = "register_type_internal", required = 3, visibility = PRIVATE)
+  public IRubyObject registerType(ThreadContext ctx, IRubyObject type, IRubyObject mod, IRubyObject proc) {
     testFrozen("MessagePack::Packer");
 
     Ruby runtime = ctx.runtime;
-    IRubyObject type = args[0];
-    IRubyObject mod = args[1];
-
-    IRubyObject arg;
-    IRubyObject proc;
-    if (args.length == 2) {
-      if (! block.isGiven()) {
-        throw runtime.newLocalJumpErrorNoBlock();
-      }
-      proc = block.getProcObject();
-      arg = proc;
-    } else if (args.length == 3) {
-      arg = args[2];
-      proc = arg.callMethod(ctx, "to_proc");
-    } else {
-      throw runtime.newArgumentError(String.format("wrong number of arguments (%d for 2..3)", 2 + args.length));
-    }
 
     long typeId = ((RubyFixnum) type).getLongValue();
     if (typeId < -128 || typeId > 127) {
@@ -126,7 +109,7 @@ public IRubyObject registerType(ThreadContext ctx, IRubyObject[] args, final Blo
     }
     RubyModule extModule = (RubyModule) mod;
 
-    registry.put(extModule, (int) typeId, false, proc, arg, null, null);
+    registry.put(extModule, (int) typeId, false, proc, null);
 
     if (extModule == runtime.getSymbol() && !proc.isNil()) {
       encoder.hasSymbolExtType = true;

ext/java/org/msgpack/jruby/Unpacker.java

@@ -126,39 +126,23 @@ public IRubyObject registeredTypesInternal(ThreadContext ctx) {
     return registry.toInternalUnpackerRegistry(ctx);
   }
 
-  @JRubyMethod(name = "register_type", required = 1, optional = 2)
-  public IRubyObject registerType(ThreadContext ctx, IRubyObject[] args, final Block block) {
+  @JRubyMethod(name = "register_type_internal", required = 3, visibility = PRIVATE)
+  public IRubyObject registerTypeInternal(ThreadContext ctx, IRubyObject type, IRubyObject mod, IRubyObject proc) {
     testFrozen("MessagePack::Unpacker");
 
     Ruby runtime = ctx.runtime;
-    IRubyObject type = args[0];
-
-    RubyModule extModule;
-    IRubyObject arg;
-    IRubyObject proc;
-    if (args.length == 1) {
-      if (! block.isGiven()) {
-        throw runtime.newLocalJumpErrorNoBlock();
-      }
-      proc = RubyProc.newProc(runtime, block, block.type);
-      if (proc == null)
-        System.err.println("proc from Block is null");
-      arg = proc;
-      extModule = null;
-    } else if (args.length == 3) {
-      extModule = (RubyModule) args[1];
-      arg = args[2];
-      proc = extModule.method(arg);
-    } else {
-      throw runtime.newArgumentError(String.format("wrong number of arguments (%d for 1 or 3)", 2 + args.length));
-    }
 
     long typeId = ((RubyFixnum) type).getLongValue();
     if (typeId < -128 || typeId > 127) {
       throw runtime.newRangeError(String.format("integer %d too big to convert to `signed char'", typeId));
     }
 
-    registry.put(extModule, (int) typeId, false, null, null, proc, arg);
+    RubyModule extModule = null;
+    if (mod != runtime.getNil()) {
+      extModule = (RubyModule)mod;
+    }
+
+    registry.put(extModule, (int) typeId, false, null, proc);
     return runtime.getNil();
   }
 

ext/msgpack/extconf.rb

@@ -4,6 +4,7 @@
 have_header("st.h")
 have_func("rb_enc_interned_str", "ruby.h") # Ruby 3.0+
 have_func("rb_hash_new_capa", "ruby.h") # Ruby 3.2+
+have_func("rb_proc_call_with_block", "ruby.h") # CRuby (TruffleRuby doesn't have it)
 
 append_cflags([
   "-fvisibility=hidden",

ext/msgpack/factory_class.c

@@ -187,7 +187,7 @@ static VALUE Factory_registered_types_internal(VALUE self)
     VALUE uk_mapping = rb_hash_new();
     if (fc->ukrg) {
         for(int i=0; i < 256; i++) {
-            if(fc->ukrg->array[i] != Qnil) {
+            if(!NIL_P(fc->ukrg->array[i])) {
                 rb_hash_aset(uk_mapping, INT2FIX(i - 128), fc->ukrg->array[i]);
             }
         }
@@ -200,72 +200,35 @@ static VALUE Factory_registered_types_internal(VALUE self)
     );
 }
 
-static VALUE Factory_register_type(int argc, VALUE* argv, VALUE self)
+static VALUE Factory_register_type_internal(VALUE self, VALUE rb_ext_type, VALUE ext_module, VALUE options)
 {
     msgpack_factory_t *fc = Factory_get(self);
 
-    int ext_type;
-    int flags = 0;
-    VALUE ext_module;
-    VALUE options = Qnil;
-    VALUE packer_arg, unpacker_arg;
-    VALUE packer_proc, unpacker_proc;
+    Check_Type(rb_ext_type, T_FIXNUM);
 
-    if (OBJ_FROZEN(self)) {
-        rb_raise(rb_eFrozenError, "can't modify frozen MessagePack::Factory");
+    if(rb_type(ext_module) != T_MODULE && rb_type(ext_module) != T_CLASS) {
+        rb_raise(rb_eArgError, "expected Module/Class but found %s.", rb_obj_classname(ext_module));
     }
 
-    switch (argc) {
-    case 2:
-        /* register_type(0x7f, Time) */
-        packer_arg = ID2SYM(rb_intern("to_msgpack_ext"));
-        unpacker_arg = ID2SYM(rb_intern("from_msgpack_ext"));
-        break;
-    case 3:
-        /* register_type(0x7f, Time, packer: proc-like, unpacker: proc-like) */
-        options = argv[2];
-        if(rb_type(options) != T_HASH) {
-            rb_raise(rb_eArgError, "expected Hash but found %s.", rb_obj_classname(options));
-        }
+    int flags = 0;
 
-        packer_arg = rb_hash_aref(options, ID2SYM(rb_intern("packer")));
-        unpacker_arg = rb_hash_aref(options, ID2SYM(rb_intern("unpacker")));
-        break;
-    default:
-        rb_raise(rb_eArgError, "wrong number of arguments (%d for 2..3)", argc);
+    VALUE packer_proc = Qnil;
+    VALUE unpacker_proc = Qnil;
+    if(!NIL_P(options)) {
+        Check_Type(options, T_HASH);
+        packer_proc = rb_hash_aref(options, ID2SYM(rb_intern("packer")));
+        unpacker_proc = rb_hash_aref(options, ID2SYM(rb_intern("unpacker")));
     }
 
-    if (options != Qnil) {
-        Check_Type(options, T_HASH);
+    if (OBJ_FROZEN(self)) {
+        rb_raise(rb_eFrozenError, "can't modify frozen MessagePack::Factory");
     }
 
-    ext_type = NUM2INT(argv[0]);
+    int ext_type = NUM2INT(rb_ext_type);
     if(ext_type < -128 || ext_type > 127) {
         rb_raise(rb_eRangeError, "integer %d too big to convert to `signed char'", ext_type);
     }
 
-    ext_module = argv[1];
-    if(rb_type(ext_module) != T_MODULE && rb_type(ext_module) != T_CLASS) {
-        rb_raise(rb_eArgError, "expected Module/Class but found %s.", rb_obj_classname(ext_module));
-    }
-
-    packer_proc = Qnil;
-    unpacker_proc = Qnil;
-
-    if(packer_arg != Qnil) {
-        packer_proc = rb_funcall(packer_arg, rb_intern("to_proc"), 0);
-    }
-
-    if(unpacker_arg != Qnil) {
-        if(rb_type(unpacker_arg) == T_SYMBOL || rb_type(unpacker_arg) == T_STRING) {
-            unpacker_proc = rb_obj_method(ext_module, unpacker_arg);
-        } else if (rb_respond_to(unpacker_arg, rb_intern("call"))) {
-            unpacker_proc = unpacker_arg;
-        } else {
-            unpacker_proc = rb_funcall(unpacker_arg, rb_intern("method"), 1, ID2SYM(rb_intern("call")));
-        }
-    }
-
     if(ext_module == rb_cSymbol) {
         if(NIL_P(options) || RTEST(rb_hash_aref(options, ID2SYM(rb_intern("packer"))))) {
             fc->has_symbol_ext_type = true;
@@ -289,8 +252,8 @@ static VALUE Factory_register_type(int argc, VALUE* argv, VALUE self)
         }
     }
 
-    msgpack_packer_ext_registry_put(self, &fc->pkrg, ext_module, ext_type, flags, packer_proc, packer_arg);
-    msgpack_unpacker_ext_registry_put(self, &fc->ukrg, ext_module, ext_type, flags, unpacker_proc, unpacker_arg);
+    msgpack_packer_ext_registry_put(self, &fc->pkrg, ext_module, ext_type, flags, packer_proc);
+    msgpack_unpacker_ext_registry_put(self, &fc->ukrg, ext_module, ext_type, flags, unpacker_proc);
 
     return Qnil;
 }
@@ -309,5 +272,5 @@ void MessagePack_Factory_module_init(VALUE mMessagePack)
     rb_define_method(cMessagePack_Factory, "unpacker", MessagePack_Factory_unpacker, -1);
 
     rb_define_private_method(cMessagePack_Factory, "registered_types_internal", Factory_registered_types_internal, 0);
-    rb_define_method(cMessagePack_Factory, "register_type", Factory_register_type, -1);
+    rb_define_private_method(cMessagePack_Factory, "register_type_internal", Factory_register_type_internal, 3);
 }

ext/msgpack/packer.c

@@ -18,15 +18,9 @@
 
 #include "packer.h"
 
-static ID s_call;
-
-void msgpack_packer_static_init(void)
-{
-    s_call = rb_intern("call");
-}
-
-void msgpack_packer_static_destroy(void)
-{ }
+#if !defined(HAVE_RB_PROC_CALL_WITH_BLOCK)
+#define rb_proc_call_with_block(recv, argc, argv, block) rb_funcallv(recv, rb_intern("call"), argc, argv)
+#endif
 
 void msgpack_packer_init(msgpack_packer_t* pk)
 {
@@ -100,14 +94,13 @@ struct msgpack_call_proc_args_t;
 typedef struct msgpack_call_proc_args_t msgpack_call_proc_args_t;
 struct msgpack_call_proc_args_t {
     VALUE proc;
-    VALUE arg;
-    VALUE packer;
+    VALUE args[2];
 };
 
 VALUE msgpack_packer_try_calling_proc(VALUE value)
 {
     msgpack_call_proc_args_t *args = (msgpack_call_proc_args_t *)value;
-    return rb_funcall(args->proc, s_call, 2, args->arg, args->packer);
+    return rb_proc_call_with_block(args->proc, 2, args->args, Qnil);
 }
 
 bool msgpack_packer_try_write_with_ext_type_lookup(msgpack_packer_t* pk, VALUE v)
@@ -125,7 +118,7 @@ bool msgpack_packer_try_write_with_ext_type_lookup(msgpack_packer_t* pk, VALUE v
         msgpack_buffer_init(PACKER_BUFFER_(pk));
 
         int exception_occured = 0;
-        msgpack_call_proc_args_t args = { proc, v, pk->to_msgpack_arg };
+        msgpack_call_proc_args_t args = { proc, { v, pk->to_msgpack_arg } };
         rb_protect(msgpack_packer_try_calling_proc, (VALUE)&args, &exception_occured);
 
         if (exception_occured) {
@@ -140,7 +133,7 @@ bool msgpack_packer_try_write_with_ext_type_lookup(msgpack_packer_t* pk, VALUE v
             msgpack_packer_write_ext(pk, ext_type, payload);
         }
     } else {
-        VALUE payload = rb_funcall(proc, s_call, 1, v);
+        VALUE payload = rb_proc_call_with_block(proc, 1, &v, Qnil);
         StringValue(payload);
         msgpack_packer_write_ext(pk, ext_type, payload);
     }