* hash.c (rb_hash_delete_key): delete the entry without calling block.

* hash.c (rb_hash_shift): should consider iter_lev too.
	* hash.c (delete_if_i): use rb_hash_delete_key() so that the block
	  isn't called twice.  [ruby-core:11556]


git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_1_8_6@13224 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

ChangeLog

@@ -1,3 +1,12 @@
+Wed Aug 22 12:40:15 2007  Nobuyoshi Nakada  <nobu@ruby-lang.org>
+
+	* hash.c (rb_hash_delete_key): delete the entry without calling block.
+
+	* hash.c (rb_hash_shift): should consider iter_lev too.
+
+	* hash.c (delete_if_i): use rb_hash_delete_key() so that the block
+	  isn't called twice.  [ruby-core:11556]
+
 Sun Arg 12 03:56:30 2007  Masatoshi SEKI  <m_seki@mva.biglobe.ne.jp>
 
 	* lib/rinda/tuplespace.rb: fix Rinda::TupleSpace keeper thread bug.

hash.c

@@ -590,6 +590,23 @@ index_i(key, value, args)
     return ST_CONTINUE;
 }
 
+static VALUE
+rb_hash_delete_key(hash, key)
+    VALUE hash, key;
+{
+    st_data_t ktmp = (st_data_t)key, val;
+
+    if (RHASH(hash)->iter_lev > 0) {
+	if (st_delete_safe(RHASH(hash)->tbl, &ktmp, &val, Qundef)) {
+	    FL_SET(hash, HASH_DELETED);
+	    return (VALUE)val;
+	}
+    }
+    else if (st_delete(RHASH(hash)->tbl, &ktmp, &val))
+	return (VALUE)val;
+    return Qundef;
+}
+
 /*
  *  call-seq:
  *     hsh.index(value)    => key
@@ -669,14 +686,8 @@ rb_hash_delete(hash, key)
     VALUE val;
 
     rb_hash_modify(hash);
-    if (RHASH(hash)->iter_lev > 0) {
-	if (st_delete_safe(RHASH(hash)->tbl, (st_data_t*)&key, &val, Qundef)) {
-	    FL_SET(hash, HASH_DELETED);
-	    return val;
-	}
-    }
-    else if (st_delete(RHASH(hash)->tbl, (st_data_t*)&key, &val))
-	return val;
+    val = rb_hash_delete_key(hash, key);
+    if (val != Qundef) return val;
     if (rb_block_given_p()) {
 	return rb_yield(key);
     }
@@ -684,7 +695,6 @@ rb_hash_delete(hash, key)
 }
 
 struct shift_var {
-    int stop;
     VALUE key;
     VALUE val;
 };
@@ -695,13 +705,23 @@ shift_i(key, value, var)
     struct shift_var *var;
 {
     if (key == Qundef) return ST_CONTINUE;
-    if (var->stop) return ST_STOP;
-    var->stop = 1;
+    if (var->key != Qundef) return ST_STOP;
     var->key = key;
     var->val = value;
     return ST_DELETE;
 }
 
+static int
+shift_i_safe(key, value, var)
+    VALUE key, value;
+    struct shift_var *var;
+{
+    if (key == Qundef) return ST_CONTINUE;
+    var->key = key;
+    var->val = value;
+    return ST_STOP;
+}
+
 /*
  *  call-seq:
  *     hsh.shift -> anArray or obj
@@ -722,10 +742,21 @@ rb_hash_shift(hash)
     struct shift_var var;
 
     rb_hash_modify(hash);
-    var.stop = 0;
-    rb_hash_foreach(hash, shift_i, (st_data_t)&var);
+    var.key = Qundef;
+    if (RHASH(hash)->iter_lev > 0) {
+	rb_hash_foreach(hash, shift_i_safe, (st_data_t)&var);
+	if (var.key != Qundef) {
+	    st_data_t key = var.key;
+	    if (st_delete_safe(RHASH(hash)->tbl, &key, 0, Qundef)) {
+		FL_SET(hash, HASH_DELETED);
+	    }
+	}
+    }
+    else {
+	rb_hash_foreach(hash, shift_i, (st_data_t)&var);
+    }
 
-    if (var.stop) {
+    if (var.key != Qundef) {
 	return rb_assoc_new(var.key, var.val);
     }
     else if (FL_TEST(hash, HASH_PROC_DEFAULT)) {
@@ -742,7 +773,7 @@ delete_if_i(key, value, hash)
 {
     if (key == Qundef) return ST_CONTINUE;
     if (RTEST(rb_yield_values(2, key, value))) {
-	rb_hash_delete(hash, key);
+	rb_hash_delete_key(hash, key);
     }
     return ST_CONTINUE;
 }

test/ruby/test_hash.rb

@@ -1,6 +1,7 @@
 require 'test/unit'
 
 class TestHash < Test::Unit::TestCase
+
   def test_hash
     x = {1=>2, 2=>4, 3=>6}
     y = {1, 2, 2, 4, 3, 6}
@@ -71,4 +72,567 @@ class TestHash < Test::Unit::TestCase
     assert_equal(44, x[22])
     assert_equal(0, $z)
   end
+
+  # From rubicon
+
+  def setup
+    @cls = Hash
+    @h = @cls[
+      1 => 'one', 2 => 'two', 3 => 'three',
+      self => 'self', true => 'true', nil => 'nil',
+      'nil' => nil
+    ]
+  end
+
+  def test_s_AREF
+    h = @cls["a" => 100, "b" => 200]
+    assert_equal(100, h['a'])
+    assert_equal(200, h['b'])
+    assert_nil(h['c'])
+
+    h = @cls.[]("a" => 100, "b" => 200)
+    assert_equal(100, h['a'])
+    assert_equal(200, h['b'])
+    assert_nil(h['c'])
+  end
+
+  def test_s_new
+    h = @cls.new
+    assert_instance_of(@cls, h)
+    assert_nil(h.default)
+    assert_nil(h['spurious'])
+
+    h = @cls.new('default')
+    assert_instance_of(@cls, h)
+    assert_equal('default', h.default)
+    assert_equal('default', h['spurious'])
+    
+  end
+
+  def test_AREF # '[]'
+    t = Time.now
+    h = @cls[
+      1 => 'one', 2 => 'two', 3 => 'three',
+      self => 'self', t => 'time', nil => 'nil',
+      'nil' => nil
+    ]
+
+    assert_equal('one',   h[1])
+    assert_equal('two',   h[2])
+    assert_equal('three', h[3])
+    assert_equal('self',  h[self])
+    assert_equal('time',  h[t])
+    assert_equal('nil',   h[nil])
+    assert_equal(nil,     h['nil'])
+    assert_equal(nil,     h['koala'])
+
+    h1 = h.dup
+    h1.default = :default
+
+    assert_equal('one',    h1[1])
+    assert_equal('two',    h1[2])
+    assert_equal('three',  h1[3])
+    assert_equal('self',   h1[self])
+    assert_equal('time',   h1[t])
+    assert_equal('nil',    h1[nil])
+    assert_equal(nil,      h1['nil'])
+    assert_equal(:default, h1['koala'])
+
+
+  end
+
+  def test_ASET # '[]='
+    t = Time.now
+    h = @cls.new
+    h[1]     = 'one'
+    h[2]     = 'two'
+    h[3]     = 'three'
+    h[self]  = 'self'
+    h[t]     = 'time'
+    h[nil]   = 'nil'
+    h['nil'] = nil
+    assert_equal('one',   h[1])
+    assert_equal('two',   h[2])
+    assert_equal('three', h[3])
+    assert_equal('self',  h[self])
+    assert_equal('time',  h[t])
+    assert_equal('nil',   h[nil])
+    assert_equal(nil,     h['nil'])
+    assert_equal(nil,     h['koala'])
+
+    h[1] = 1
+    h[nil] = 99
+    h['nil'] = nil
+    z = [1,2]
+    h[z] = 256
+    assert_equal(1,       h[1])
+    assert_equal('two',   h[2])
+    assert_equal('three', h[3])
+    assert_equal('self',  h[self])
+    assert_equal('time',  h[t])
+    assert_equal(99,      h[nil])
+    assert_equal(nil,     h['nil'])
+    assert_equal(nil,     h['koala'])
+    assert_equal(256,     h[z])
+  end
+
+  def test_EQUAL # '=='
+    h1 = @cls[ "a" => 1, "c" => 2 ]
+    h2 = @cls[ "a" => 1, "c" => 2, 7 => 35 ]
+    h3 = @cls[ "a" => 1, "c" => 2, 7 => 35 ]
+    h4 = @cls[ ]
+    assert(h1 == h1)
+    assert(h2 == h2)
+    assert(h3 == h3)
+    assert(h4 == h4)
+    assert(!(h1 == h2))
+    assert(h2 == h3)
+    assert(!(h3 == h4))
+  end
+
+  def test_clear
+    assert(@h.size > 0)
+    @h.clear
+    assert_equal(0, @h.size)
+    assert_nil(@h[1])
+  end
+
+  def test_clone
+    for taint in [ false, true ]
+      for frozen in [ false, true ]
+        a = @h.clone
+        a.taint  if taint
+        a.freeze if frozen
+        b = a.clone
+
+        assert_equal(a, b)
+        assert(a.__id__ != b.__id__)
+        assert_equal(a.frozen?, b.frozen?)
+        assert_equal(a.tainted?, b.tainted?)
+      end
+    end
+  end
+
+  def test_default
+    assert_nil(@h.default)
+    h = @cls.new(:xyzzy)
+    assert_equal(:xyzzy, h.default)
+  end
+
+  def test_default=
+    assert_nil(@h.default)
+    @h.default = :xyzzy
+    assert_equal(:xyzzy, @h.default)
+  end
+
+  def test_delete
+    h1 = @cls[ 1 => 'one', 2 => 'two', true => 'true' ]
+    h2 = @cls[ 1 => 'one', 2 => 'two' ]
+    h3 = @cls[ 2 => 'two' ]
+
+    assert_equal('true', h1.delete(true))
+    assert_equal(h2, h1)
+
+    assert_equal('one', h1.delete(1))
+    assert_equal(h3, h1)
+
+    assert_equal('two', h1.delete(2))
+    assert_equal(@cls[], h1)
+
+    assert_nil(h1.delete(99))
+    assert_equal(@cls[], h1)
+
+    assert_equal('default 99', h1.delete(99) {|i| "default #{i}" })
+  end
+
+  def test_delete_if
+    base = @cls[ 1 => 'one', 2 => false, true => 'true', 'cat' => 99 ]
+    h1   = @cls[ 1 => 'one', 2 => false, true => 'true' ]
+    h2   = @cls[ 2 => false, 'cat' => 99 ]
+    h3   = @cls[ 2 => false ]
+
+    h = base.dup
+    assert_equal(h, h.delete_if { false })
+    assert_equal(@cls[], h.delete_if { true })
+
+    h = base.dup
+    assert_equal(h1, h.delete_if {|k,v| k.instance_of?(String) })
+    assert_equal(h1, h)
+
+    h = base.dup
+    assert_equal(h2, h.delete_if {|k,v| v.instance_of?(String) })
+    assert_equal(h2, h)
+
+    h = base.dup
+    assert_equal(h3, h.delete_if {|k,v| v })
+    assert_equal(h3, h)
+
+    h = base.dup
+    n = 0
+    h.delete_if {|*a|
+      n += 1
+      assert_equal(2, a.size)
+      assert_equal(base[a[0]], a[1])
+      h.shift
+      true
+    }
+    assert_equal(base.size, n)
+  end
+
+  def test_dup
+    for taint in [ false, true ]
+      for frozen in [ false, true ]
+        a = @h.dup
+        a.taint  if taint
+        a.freeze if frozen
+        b = a.dup
+
+        assert_equal(a, b)
+        assert(a.__id__ != b.__id__)
+        assert_equal(false, b.frozen?)
+        assert_equal(a.tainted?, b.tainted?)
+      end
+    end
+  end
+
+  def test_each
+    count = 0
+    @cls[].each { |k, v| count + 1 }
+    assert_equal(0, count)
+
+    h = @h
+    h.each do |k, v|
+      assert_equal(v, h.delete(k))
+    end
+    assert_equal(@cls[], h)
+  end
+
+  def test_each_key
+    count = 0
+    @cls[].each_key { |k| count + 1 }
+    assert_equal(0, count)
+
+    h = @h
+    h.each_key do |k|
+      h.delete(k)
+    end
+    assert_equal(@cls[], h)
+  end
+
+  def test_each_pair
+    count = 0
+    @cls[].each_pair { |k, v| count + 1 }
+    assert_equal(0, count)
+
+    h = @h
+    h.each_pair do |k, v|
+      assert_equal(v, h.delete(k))
+    end
+    assert_equal(@cls[], h)
+  end
+
+  def test_each_value
+    res = []
+    @cls[].each_value { |v| res << v }
+    assert_equal(0, [].length)
+
+    @h.each_value { |v| res << v }
+    assert_equal(0, [].length)
+
+    expected = []
+    @h.each { |k, v| expected << v }
+
+    assert_equal([], expected - res)
+    assert_equal([], res - expected)
+  end
+
+  def test_empty?
+    assert(@cls[].empty?)
+    assert(!@h.empty?)
+  end
+
+  def test_fetch
+    assert_raise(IndexError) { @cls[].fetch(1) }
+    assert_raise(IndexError) { @h.fetch('gumby') }
+    assert_equal('gumbygumby', @h.fetch('gumby') {|k| k * 2 })
+    assert_equal('pokey', @h.fetch('gumby', 'pokey'))
+
+    assert_equal('one', @h.fetch(1))
+    assert_equal(nil, @h.fetch('nil'))
+    assert_equal('nil', @h.fetch(nil))
+  end
+
+  def test_key?
+    assert(!@cls[].key?(1))
+    assert(!@cls[].key?(nil))
+    assert(@h.key?(nil))
+    assert(@h.key?(1))
+    assert(!@h.key?('gumby'))
+  end
+
+  def test_value?
+    assert(!@cls[].value?(1))
+    assert(!@cls[].value?(nil))
+    assert(@h.value?('one'))
+    assert(@h.value?(nil))
+    assert(!@h.value?('gumby'))
+  end
+
+  def test_include?
+    assert(!@cls[].include?(1))
+    assert(!@cls[].include?(nil))
+    assert(@h.include?(nil))
+    assert(@h.include?(1))
+    assert(!@h.include?('gumby'))
+  end
+
+  def test_values_at
+    res = @h.values_at('dog', 'cat', 'horse')
+    assert(res.length == 3)
+    assert_equal([nil, nil, nil], res)
+
+    res = @h.values_at
+    assert(res.length == 0)
+
+    res = @h.values_at(3, 2, 1, nil)
+    assert_equal 4, res.length
+    assert_equal %w( three two one nil ), res
+
+    res = @h.values_at(3, 99, 1, nil)
+    assert_equal 4, res.length
+    assert_equal ['three', nil, 'one', 'nil'], res
+  end
+
+
+  def test_invert
+    h = @h.invert
+    assert_equal(1, h['one'])
+    assert_equal(true, h['true'])
+    assert_equal(nil,  h['nil'])
+
+    h.each do |k, v|
+      assert(@h.key?(v))    # not true in general, but works here
+    end
+
+    h = @cls[ 'a' => 1, 'b' => 2, 'c' => 1].invert
+    assert_equal(2, h.length)
+    assert(h[1] == 'a' || h[1] == 'c')
+    assert_equal('b', h[2])
+  end
+
+  def test_key?
+    assert(!@cls[].key?(1))
+    assert(!@cls[].key?(nil))
+    assert(@h.key?(nil))
+    assert(@h.key?(1))
+    assert(!@h.key?('gumby'))
+  end
+
+  def test_keys
+    assert_equal([], @cls[].keys)
+
+    keys = @h.keys
+    expected = []
+    @h.each { |k, v| expected << k }
+    assert_equal([], keys - expected)
+    assert_equal([], expected - keys)
+  end
+
+  def test_length
+    assert_equal(0, @cls[].length)
+    assert_equal(7, @h.length)
+  end
+
+  def test_member?
+    assert(!@cls[].member?(1))
+    assert(!@cls[].member?(nil))
+    assert(@h.member?(nil))
+    assert(@h.member?(1))
+    assert(!@h.member?('gumby'))
+  end
+
+  def test_rehash
+    a = [ "a", "b" ]
+    c = [ "c", "d" ]
+    h = @cls[ a => 100, c => 300 ]
+    assert_equal(100, h[a])
+    a[0] = "z"
+    assert_nil(h[a])
+    h.rehash
+    assert_equal(100, h[a])
+  end
+
+  def test_reject
+    base = @cls[ 1 => 'one', 2 => false, true => 'true', 'cat' => 99 ]
+    h1   = @cls[ 1 => 'one', 2 => false, true => 'true' ]
+    h2   = @cls[ 2 => false, 'cat' => 99 ]
+    h3   = @cls[ 2 => false ]
+
+    h = base.dup
+    assert_equal(h, h.reject { false })
+    assert_equal(@cls[], h.reject { true })
+
+    h = base.dup
+    assert_equal(h1, h.reject {|k,v| k.instance_of?(String) })
+
+    assert_equal(h2, h.reject {|k,v| v.instance_of?(String) })
+
+    assert_equal(h3, h.reject {|k,v| v })
+    assert_equal(base, h)
+  end
+
+  def test_reject!
+    base = @cls[ 1 => 'one', 2 => false, true => 'true', 'cat' => 99 ]
+    h1   = @cls[ 1 => 'one', 2 => false, true => 'true' ]
+    h2   = @cls[ 2 => false, 'cat' => 99 ]
+    h3   = @cls[ 2 => false ]
+
+    h = base.dup
+    assert_equal(nil, h.reject! { false })
+    assert_equal(@cls[],  h.reject! { true })
+
+    h = base.dup
+    assert_equal(h1, h.reject! {|k,v| k.instance_of?(String) })
+    assert_equal(h1, h)
+
+    h = base.dup
+    assert_equal(h2, h.reject! {|k,v| v.instance_of?(String) })
+    assert_equal(h2, h)
+
+    h = base.dup
+    assert_equal(h3, h.reject! {|k,v| v })
+    assert_equal(h3, h)
+  end
+
+  def test_replace
+    h = @cls[ 1 => 2, 3 => 4 ]
+    h1 = h.replace(@cls[ 9 => 8, 7 => 6 ])
+    assert_equal(h, h1)
+    assert_equal(8, h[9])
+    assert_equal(6, h[7])
+    assert_nil(h[1])
+    assert_nil(h[2])
+  end
+
+  def test_shift
+    h = @h.dup
+    
+    @h.length.times {
+      k, v = h.shift
+      assert(@h.key?(k))
+      assert_equal(@h[k], v)
+    }
+
+    assert_equal(0, h.length)
+  end
+
+  def test_size
+    assert_equal(0, @cls[].length)
+    assert_equal(7, @h.length)
+  end
+
+  def test_sort
+    h = @cls[].sort
+    assert_equal([], h)
+
+    h = @cls[ 1 => 1, 2 => 1 ].sort
+    assert_equal([[1,1], [2,1]], h)
+
+    h = @cls[ 'cat' => 'feline', 'ass' => 'asinine', 'bee' => 'beeline' ]
+    h1 = h.sort
+    assert_equal([ %w(ass asinine), %w(bee beeline), %w(cat feline)], h1)
+  end
+
+  def test_store
+    t = Time.now
+    h = @cls.new
+    h.store(1, 'one')
+    h.store(2, 'two')
+    h.store(3, 'three')
+    h.store(self, 'self')
+    h.store(t,  'time')
+    h.store(nil, 'nil')
+    h.store('nil', nil)
+    assert_equal('one',   h[1])
+    assert_equal('two',   h[2])
+    assert_equal('three', h[3])
+    assert_equal('self',  h[self])
+    assert_equal('time',  h[t])
+    assert_equal('nil',   h[nil])
+    assert_equal(nil,     h['nil'])
+    assert_equal(nil,     h['koala'])
+
+    h.store(1, 1)
+    h.store(nil,  99)
+    h.store('nil', nil)
+    assert_equal(1,       h[1])
+    assert_equal('two',   h[2])
+    assert_equal('three', h[3])
+    assert_equal('self',  h[self])
+    assert_equal('time',  h[t])
+    assert_equal(99,      h[nil])
+    assert_equal(nil,     h['nil'])
+    assert_equal(nil,     h['koala'])
+  end
+
+  def test_to_a
+    assert_equal([], @cls[].to_a)
+    assert_equal([[1,2]], @cls[ 1=>2 ].to_a)
+    a = @cls[ 1=>2, 3=>4, 5=>6 ].to_a
+    assert_equal([1,2], a.delete([1,2]))
+    assert_equal([3,4], a.delete([3,4]))
+    assert_equal([5,6], a.delete([5,6]))
+    assert_equal(0, a.length)
+  end
+
+  def test_to_hash
+    h = @h.to_hash
+    assert_equal(@h, h)
+  end
+
+  def test_to_s
+    h = @cls[ 1 => 2, "cat" => "dog", 1.5 => :fred ]
+    assert_equal(h.to_a.join, h.to_s)
+    $, = ":"
+    assert_equal(h.to_a.join, h.to_s)
+    h = @cls[]
+    assert_equal(h.to_a.join, h.to_s)
+    $, = nil
+  end
+
+  def test_update
+    h1 = @cls[ 1 => 2, 2 => 3, 3 => 4 ]
+    h2 = @cls[ 2 => 'two', 4 => 'four' ]
+
+    ha = @cls[ 1 => 2, 2 => 'two', 3 => 4, 4 => 'four' ]
+    hb = @cls[ 1 => 2, 2 => 3, 3 => 4, 4 => 'four' ]
+
+    assert_equal(ha, h1.update(h2))
+    assert_equal(ha, h1)
+
+    h1 = @cls[ 1 => 2, 2 => 3, 3 => 4 ]
+    h2 = @cls[ 2 => 'two', 4 => 'four' ]
+
+    assert_equal(hb, h2.update(h1))
+    assert_equal(hb, h2)
+  end
+
+  def test_value?
+    assert(!@cls[].value?(1))
+    assert(!@cls[].value?(nil))
+    assert(@h.value?(nil))
+    assert(@h.value?('one'))
+    assert(!@h.value?('gumby'))
+  end
+
+  def test_values
+    assert_equal([], @cls[].values)
+
+    vals = @h.values
+    expected = []
+    @h.each { |k, v| expected << v }
+    assert_equal([], vals - expected)
+    assert_equal([], expected - vals)
+  end
+
 end

version.h

@@ -2,7 +2,7 @@
 #define RUBY_RELEASE_DATE "2007-08-22"
 #define RUBY_VERSION_CODE 186
 #define RUBY_RELEASE_CODE 20070822
-#define RUBY_PATCHLEVEL 88
+#define RUBY_PATCHLEVEL 89
 
 #define RUBY_VERSION_MAJOR 1
 #define RUBY_VERSION_MINOR 8