Skip to content
This repository

Add initialize_dup and initialize_clone #1910

Closed
wants to merge 4 commits into from

3 participants

Carlos Galdino Brian Shirai Dirkjan Bussink
Carlos Galdino
Collaborator

This pull request isn't complete yet.

I opened because I need some feedback on how to change Kernel#dup and Kernel#clone since each method needs to call #initialize_dup and #initialize_clone, respectively.

Kernel#dup and #clone are defined in kernel/alpha.rb and the call to #initialize_dup should only occur on 1.9 mode. My question is, how should I proceed?

Copy the code for #clone and #dup to kernel/common/kernel19.rb and then change it so it will call the appropriate methods? Or is there a better solution?

The same question is valid for other classes that need to add specific behavior to #dup and #clone. Most of them are defined on kernel/bootstrap but on kernel/bootstrap I see that there are files specific to 1.9 mode so the fix should be split the method definition on both imaginary_class18.rb and imaginary_class19.rb and add the correct behavior on each file. Is that right?

Thanks.

P.S.: This will fix #1889.

Brian Shirai brixen commented on the diff September 21, 2012
spec/ruby/core/kernel/initialize_clone_spec.rb
((3 lines not shown))
2 3
 
3 4
 ruby_version_is "1.9" do
4 5
   describe "Kernel#initialize_clone" do
5  
-    it "needs to be reviewed for spec completeness"
  6
+    it "calls #initialize_copy" do
  7
+      ScratchPad.clear
1
Brian Shirai Owner
brixen added a note September 21, 2012

Setup code goes in a before action.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Brian Shirai
Owner

This post has a discussion of language version-specific changes http://rubini.us/2011/10/18/contributing-to-rubinius/. Generally, you do put the methods in different files. However, we don't want to duplicate code unnecessarily.

In this case, it's a little bit complicated by having those methods defined in alpha.rb. That file has the bare minimum necessary to start loading code. However, we have analogous cases of dealing with this sort of version-specific behavior. When it involves things like the "kind" of objects or critical object reflection capabilities, we use the Rubinius::Type module. I'd suggest doing something like this:

diff --git a/kernel/alpha.rb b/kernel/alpha.rb
index fb59d54..4c2522a 100644
--- a/kernel/alpha.rb
+++ b/kernel/alpha.rb
@@ -206,9 +206,7 @@ module Kernel
 
     Rubinius.invoke_primitive :object_copy_object, copy, self
 
-    Rubinius.privately do
-      copy.initialize_copy self
-    end
+    Rubinius::Type.object_initialize_dup self, copy
     copy
   end
 
@@ -236,9 +234,7 @@ module Kernel
     Rubinius.invoke_primitive :object_copy_object, copy, self
     Rubinius.invoke_primitive :object_copy_singleton_class, copy, self
 
-    Rubinius.privately do
-      copy.initialize_copy self
-    end
+    Rubinius::Type.object_initialize_clone self, copy
 
     copy.freeze if frozen?
     copy
@@ -344,6 +340,20 @@ module Rubinius
       raise PrimitiveFailure, "MethodTable#alias primitive failed"
     end
   end
+
+  module Type
+    def self.object_initialize_dup(obj, copy)
+      Rubinius.privately do
+        copy.initialize_copy obj
+      end
+    end
+
+    def self.object_initialize_clone(obj, copy)
+      Rubinius.privately do
+        copy.initialize_copy obj
+      end
+    end
+  end
 end
 
 
@@ -734,4 +744,3 @@ end
 class Object
   include Kernel
 end
-

Define appropriate versions of Rubinius::Type.object_initialize_dup, etc. in bootstrap/type19.rb and add the appropriate Kernel, etc. methods for 1.9 mode.

Dirkjan Bussink
Owner

@carlosgaldino Do these suggestions help you get further with this?

Carlos Galdino
Collaborator
Dirkjan Bussink
Owner

Since there hasn't been much activity here, I've implemented the feature.

Dirkjan Bussink dbussink closed this November 11, 2012
Carlos Galdino
Collaborator

@dbussink Yeah, I'm sorry. I've been busy lately and didn't have any time.

Gibheer Gibheer referenced this pull request from a commit November 20, 2012
Commit has since been removed from the repository and is no longer available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
8  kernel/common/kernel19.rb
@@ -247,4 +247,12 @@ def Rational(a, b = 1)
247 247
   def <=>(other)
248 248
     self == other ? 0 : nil
249 249
   end
  250
+
  251
+  def initialize_dup(other)
  252
+    initialize_copy(other)
  253
+  end
  254
+
  255
+  def initialize_clone(other)
  256
+    initialize_copy(other)
  257
+  end
250 258
 end
12  spec/ruby/core/kernel/initialize_clone_spec.rb
... ...
@@ -1,7 +1,17 @@
1 1
 require File.expand_path('../../../spec_helper', __FILE__)
  2
+require File.expand_path('../fixtures/classes', __FILE__)
2 3
 
3 4
 ruby_version_is "1.9" do
4 5
   describe "Kernel#initialize_clone" do
5  
-    it "needs to be reviewed for spec completeness"
  6
+    it "calls #initialize_copy" do
  7
+      ScratchPad.clear
  8
+
  9
+      obj = KernelSpecs::Duplicate.new(1, :a)
  10
+      other = KernelSpecs::Duplicate.new(2, :b)
  11
+
  12
+      obj.initialize_clone(other)
  13
+
  14
+      ScratchPad.recorded.should == obj.object_id
  15
+    end
6 16
   end
7 17
 end
12  spec/ruby/core/kernel/initialize_dup_spec.rb
... ...
@@ -1,7 +1,17 @@
1 1
 require File.expand_path('../../../spec_helper', __FILE__)
  2
+require File.expand_path('../fixtures/classes', __FILE__)
2 3
 
3 4
 ruby_version_is "1.9" do
4 5
   describe "Kernel#initialize_dup" do
5  
-    it "needs to be reviewed for spec completeness"
  6
+    it "calls #initialize_copy" do
  7
+      ScratchPad.clear
  8
+
  9
+      obj = KernelSpecs::Duplicate.new(1, :a)
  10
+      other = KernelSpecs::Duplicate.new(2, :b)
  11
+
  12
+      obj.initialize_dup(other)
  13
+
  14
+      ScratchPad.recorded.should == obj.object_id
  15
+    end
6 16
   end
7 17
 end
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.