ruby-changes:66804
From: Jeremy <ko1@a...>
Date: Fri, 16 Jul 2021 02:04:30 +0900 (JST)
Subject: [ruby-changes:66804] 95f8ffa5f6 (master): Copy hash compare_by_identity setting in more cases
https://git.ruby-lang.org/ruby.git/commit/?id=95f8ffa5f6 From 95f8ffa5f6c70aa9383e1f6db02b22707c183402 Mon Sep 17 00:00:00 2001 From: Jeremy Evans <code@j...> Date: Thu, 15 Jul 2021 10:04:17 -0700 Subject: Copy hash compare_by_identity setting in more cases This makes the compare_by_identity setting always copied for the following methods: * except * merge * reject * select * slice * transform_values Some of these methods did not copy the setting, or only copied the setting if the receiver was not empty. Fixes [Bug #17757] Co-authored-by: Kenichi Kamiya <kachick1@g...> --- hash.c | 32 +++++++--- test/ruby/test_hash.rb | 169 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 193 insertions(+), 8 deletions(-) diff --git a/hash.c b/hash.c index ec41bf1..045e739 100644 --- a/hash.c +++ b/hash.c @@ -1559,6 +1559,17 @@ rb_hash_new(void) https://github.com/ruby/ruby/blob/trunk/hash.c#L1559 return hash_alloc(rb_cHash); } +static VALUE rb_hash_compare_by_id(VALUE hash); + +static VALUE +copy_compare_by_id(VALUE hash, VALUE basis) +{ + if (rb_hash_compare_by_id_p(basis)) { + return rb_hash_compare_by_id(hash); + } + return hash; +} + MJIT_FUNC_EXPORTED VALUE rb_hash_new_with_size(st_index_t size) { @@ -1588,6 +1599,12 @@ hash_copy(VALUE ret, VALUE hash) https://github.com/ruby/ruby/blob/trunk/hash.c#L1599 } static VALUE +hash_dup_with_compare_by_id(VALUE hash) +{ + return hash_copy(copy_compare_by_id(rb_hash_new(), hash), hash); +} + +static VALUE hash_dup(VALUE hash, VALUE klass, VALUE flags) { return hash_copy(hash_alloc_flags(klass, flags, RHASH_IFNONE(hash)), @@ -2597,7 +2614,7 @@ rb_hash_reject(VALUE hash) https://github.com/ruby/ruby/blob/trunk/hash.c#L2614 rb_warn("extra states are no longer copied: %+"PRIsVALUE, hash); } } - result = hash_copy(hash_alloc(rb_cHash), hash); + result = hash_dup_with_compare_by_id(hash); if (!RHASH_EMPTY_P(hash)) { rb_hash_foreach(result, delete_if_i, result); } @@ -2622,9 +2639,9 @@ rb_hash_slice(int argc, VALUE *argv, VALUE hash) https://github.com/ruby/ruby/blob/trunk/hash.c#L2639 VALUE key, value, result; if (argc == 0 || RHASH_EMPTY_P(hash)) { - return rb_hash_new(); + return copy_compare_by_id(rb_hash_new(), hash); } - result = rb_hash_new_with_size(argc); + result = copy_compare_by_id(rb_hash_new_with_size(argc), hash); for (i = 0; i < argc; i++) { key = argv[i]; @@ -2653,8 +2670,7 @@ rb_hash_except(int argc, VALUE *argv, VALUE hash) https://github.com/ruby/ruby/blob/trunk/hash.c#L2670 int i; VALUE key, result; - result = hash_alloc(rb_cHash); - hash_copy(result, hash); + result = hash_dup_with_compare_by_id(hash); for (i = 0; i < argc; i++) { key = argv[i]; @@ -2754,7 +2770,7 @@ rb_hash_select(VALUE hash) https://github.com/ruby/ruby/blob/trunk/hash.c#L2770 VALUE result; RETURN_SIZED_ENUMERATOR(hash, 0, 0, hash_enum_size); - result = hash_copy(hash_alloc(rb_cHash), hash); + result = hash_dup_with_compare_by_id(hash); if (!RHASH_EMPTY_P(hash)) { rb_hash_foreach(result, keep_if_i, result); } @@ -3348,7 +3364,7 @@ rb_hash_transform_values(VALUE hash) https://github.com/ruby/ruby/blob/trunk/hash.c#L3364 VALUE result; RETURN_SIZED_ENUMERATOR(hash, 0, 0, hash_enum_size); - result = hash_copy(hash_alloc(rb_cHash), hash); + result = hash_dup_with_compare_by_id(hash); SET_DEFAULT(result, Qnil); if (!RHASH_EMPTY_P(hash)) { @@ -4102,7 +4118,7 @@ rb_hash_update_by(VALUE hash1, VALUE hash2, rb_hash_update_func *func) https://github.com/ruby/ruby/blob/trunk/hash.c#L4118 static VALUE rb_hash_merge(int argc, VALUE *argv, VALUE self) { - return rb_hash_update(argc, argv, rb_hash_dup(self)); + return rb_hash_update(argc, argv, copy_compare_by_id(rb_hash_dup(self), self)); } static int diff --git a/test/ruby/test_hash.rb b/test/ruby/test_hash.rb index 7c83d03..028610e 100644 --- a/test/ruby/test_hash.rb +++ b/test/ruby/test_hash.rb @@ -749,6 +749,33 @@ class TestHash < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_hash.rb#L749 assert_not_send([h, :instance_variable_defined?, :@foo]) end + def test_reject_on_identhash + h = @cls[1=>2,3=>4,5=>6] + h.compare_by_identity + str1 = +'str' + str2 = +'str' + h[str1] = 1 + h[str2] = 2 + expected = {}.compare_by_identity + expected[str1] = 1 + expected[str2] = 2 + h2 = h.reject{|k,| k != 'str'} + assert_equal(expected, h2) + assert_equal(true, h2.compare_by_identity?) + h2 = h.reject{true} + assert_equal({}.compare_by_identity, h2) + assert_equal(true, h2.compare_by_identity?) + + h = @cls[] + h.compare_by_identity + h2 = h.reject{true} + assert_equal({}.compare_by_identity, h2) + assert_equal(true, h2.compare_by_identity?) + h2 = h.reject{|k,| k != 'str'} + assert_equal({}.compare_by_identity, h2) + assert_equal(true, h2.compare_by_identity?) + end + def test_reject! base = @cls[ 1 => 'one', 2 => false, true => 'true', 'cat' => 99 ] h1 = @cls[ 1 => 'one', 2 => false, true => 'true' ] @@ -1076,6 +1103,33 @@ class TestHash < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_hash.rb#L1103 assert_not_send([h, :instance_variable_defined?, :@foo]) end + def test_select_on_identhash + h = @cls[1=>2,3=>4,5=>6] + h.compare_by_identity + str1 = +'str' + str2 = +'str' + h[str1] = 1 + h[str2] = 2 + expected = {}.compare_by_identity + expected[str1] = 1 + expected[str2] = 2 + h2 = h.select{|k,| k == 'str'} + assert_equal(expected, h2) + assert_equal(true, h2.compare_by_identity?) + h2 = h.select{false} + assert_equal({}.compare_by_identity, h2) + assert_equal(true, h2.compare_by_identity?) + + h = @cls[] + h.compare_by_identity + h2 = h.select{false} + assert_equal({}.compare_by_identity, h2) + assert_equal(true, h2.compare_by_identity?) + h2 = h.select{|k,| k == 'str'} + assert_equal({}.compare_by_identity, h2) + assert_equal(true, h2.compare_by_identity?) + end + def test_select! h = @cls[1=>2,3=>4,5=>6] assert_equal(h, h.select! {|k, v| k + v >= 7 }) @@ -1100,6 +1154,33 @@ class TestHash < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_hash.rb#L1154 assert_equal({}, {}.slice) end + def test_slice_on_identhash + h = @cls[1=>2,3=>4,5=>6] + h.compare_by_identity + str1 = +'str' + str2 = +'str' + h[str1] = 1 + h[str2] = 2 + sliced = h.slice(str1, str2) + expected = {}.compare_by_identity + expected[str1] = 1 + expected[str2] = 2 + assert_equal(expected, sliced) + assert_equal(true, sliced.compare_by_identity?) + sliced = h.slice + assert_equal({}.compare_by_identity, sliced) + assert_equal(true, sliced.compare_by_identity?) + + h = @cls[] + h.compare_by_identity + sliced= h.slice + assert_equal({}.compare_by_identity, sliced) + assert_equal(true, sliced.compare_by_identity?) + sliced = h.slice(str1, str2) + assert_equal({}.compare_by_identity, sliced) + assert_equal(true, sliced.compare_by_identity?) + end + def test_except h = @cls[1=>2,3=>4,5=>6] assert_equal({5=>6}, h.except(1, 3)) @@ -1108,6 +1189,30 @@ class TestHash < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_hash.rb#L1189 assert_equal({}, {}.except) end + def test_except_on_identhash + h = @cls[1=>2,3=>4,5=>6] + h.compare_by_identity + str1 = +'str' + str2 = +'str' + h[str1] = 1 + h[str2] = 2 + excepted = h.except(str1, str2) + assert_equal({1=>2,3=>4,5=>6}.compare_by_identity, excepted) + assert_equal(true, excepted.compare_by_identity?) + excepted = h.except + assert_equal(h, excepted) + assert_equal(true, excepted.compare_by_identity?) + + h = @cls[] + h.compare_by_identity + excepted = h.except + assert_equal({}.compare_by_identity, excepted) + assert_equal(true, excepted.compare_by_identity?) + excepted = h.except(str1, str2) + assert_equal({}.compare_by_identity, excepted) + assert_equal(true, excepted.compare_by_identity?) + end + def test_filter assert_equal({3=>4,5=>6}, @cls[1=>2,3=>4,5=>6].filter {|k, v| k + v >= 7 }) @@ -1267,6 +1372,34 @@ class TestHash < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_hash.rb#L1372 assert_equal({1=>8, 2=>4, 3=>4, 5=>7}, h1.merge(h2, h3) {|k, v1, v2| k + v1 + v2 }) end + def test_merge_on_identhash + h = @cls[1=>2,3=>4,5=>6] + h.compare_by_identity + str1 = +'str' + str2 = +'str' + h[str1] = 1 + h[str2] = 2 + expected = h.dup + expected[7] = 8 + h2 = h.merge(7=>8) + assert_equal(expected, h2) + assert_equal(true, h2.compare_by_identity?) + h2 = h.merge({}) + assert_equal(h, h2) + assert_equal(true, h2.compare_by_identity?) + + h = @cls[] + h.compare_by_identity + h1 = @cls[7=>8] + h1.compare_by_identity + h2 = h.merge(7=>8) + assert_equal(h1, h2) + assert_equal(true, h2.compare_by_identity?) + h2 = h.merge({}) + assert_equal(h, h2) + assert_equal(true, h2.compare_by_identity?) + end + def test_merge! h = @cls[a: 1, b: 2, c: 3] assert_raise(FrozenError) do @@ -1758,6 +1891,24 @@ class TestHash < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_hash.rb#L1891 assert_equal({A: 1, B: 2, "c" => 3}, x.transform_keys({a: :A, b: :B, d: :D}, &:to_s)) end + def test_transform_keys_on_identhash + h = @cls[1=>2,3=>4,5=>6] + h.compare_by_identity + str1 = +'str' + str2 = +'str' + h[str1] = 1 + h[str2] = 2 + h2 = h.transform_keys(&:itself) + assert_equal(Hash[h.to_a], h2) + assert_equal(false, h2.compare_by_identity?) + + h = @cls[] + h.compare_by_identity + h2 = h.transform_keys(&:itself) (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/