Commits on Apr 2, 2020

1. Call "Class#new" over rb_class_new_instance in decoding …

   This patch has almost no change in behaviour where users have not
   patched the implementation of new on either a specific proto object
   class, or `Google::Protobuf::MessageExts::ClassMethods`. The default
   implementation of `new`, and `rb_class_new_instance` have the same
   behaviour.
   
   By default when we call `new` on a class in Ruby, it goes to the `Class`
   class's implementation:
   
   ```ruby
   class Foo
   end
   
   >> Foo.method(:new).owner
   => Class
   ```
   
   the `Class` implementation of `new` is (pseudocode, it's actually in c):
   
   ```ruby
   class Class
     def new(*args, &blk)
       instance = alloc
       instance.initialize(*args, &blk)
       instance
     end
   end
   ```
   
   `rb_class_new_instance` does the same thing, it calls down to
   [`rb_class_s_new`](https://github.com/ruby/ruby/blob/v2_5_5/object.c#L2147),
   which calls `rb_class_alloc`, then `rb_obj_call_init`.
   
   `rb_funcall` is a variadic c function for calling a ruby method on an object,
   it takes:
   
   * A `VALUE` on to which the method should be called
   * An `ID` which should be an ID of a method, usually created with `rb_intern`,
     to get an ID from a string
   * An integer, the number of arguments calling the  method with,
   * A number of `VALUE`s, to send to the method call.
   
   `rb_funcall` is the same as calling a method directly in Ruby, and will perform
   ancestor chain respecting method lookup.
   
   This means that in C extensions, if nobody has defined the `new` method on any
   classes or modules in a class's inheritance chain calling
   `rb_class_new_instance` is the same as calling `rb_funcall(klass,
   rb_intern("new"))`, *however* this removes the ability for users to define or
   monkey patch their own constructors in to the objects created by the C
   extension.
   
   In Ads, we define [`new`](https://git.io/JvFC9) on
   `Google::Protobuf::MessageExts::ClassMethods` to allow us to insert a
   monkeypatch which makes it possible to assign primitive values to wrapper type
   fields (e.g. Google::Protobuf::StringValue). The monkeypatch we apply works for
   objects that we create for the user via the `new` method. Before this commit,
   however, the patch does not work for the `decode` method, for the reasons
   outlined above. Before this commit, protobuf uses `rb_class_new_instance`.
   
   After this commit, we use `rb_funcall(klass, rb_intern("new"), 0);` to construct
   protobuf objects during decoding. While I haven't measured it this will have
   a very minor performance impact for decoding (`rb_funcall` will have to go to the
   method cache, which `rb_class_new_instance` will not). This does however do
   the "more rubyish" thing of respecting the protobuf object's inheritance chain
   to construct them during decoding.
   
   I have run both Ads and Cloud's test suites for Ruby libraries against this
   patch, as well as the protobuf Ruby gem's test suite locally.

   

   fables-tales committed Apr 2, 2020
   Copy the full SHA

   60d5868 View commit details

   Browse the repository at this point in the history