Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Hash#{update, merge!} ensure the receiver modifiable [Bug #17736] #4298

Closed
wants to merge 2 commits into from
Closed

Hash#{update, merge!} ensure the receiver modifiable [Bug #17736] #4298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d977810
Add Hash#{update, merge!} test to ensure receiver modifiable in block
kachick Mar 20, 2021
12470e2
Hash#{update, merge!} ensure the receiver modifiable [Bug #17736]
kachick Mar 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion hash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1039,6 +1039,7 @@ ar_update(VALUE hash, st_data_t key,
}
old_key = key;
retval = (*func)(&key, &value, arg, existing);
rb_hash_modify(hash);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ar_update is used only for small hashes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😮 Oh... I don't know it. Thank you for letting me know! 🙏

/* pair can be invalid here because of theap */

switch (retval) {
Expand Down Expand Up @@ -1631,7 +1632,7 @@ rb_hash_tbl(VALUE hash, const char *file, int line)
return rb_hash_tbl_raw(hash, file, line);
}

static void
void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this function need to be public?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling in ar_update makes compile error without this change, and

ruby/array.c

Lines 605 to 610 in db0ad48

void
rb_ary_modify(VALUE ary)
{
rb_ary_modify_check(ary);
rb_ary_cancel_sharing(ary);
}
looks public, so changed 🙇

rb_hash_modify(VALUE hash)
{
rb_hash_modify_check(hash);
Expand Down
1 change: 1 addition & 0 deletions include/ruby/internal/intern/hash.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ VALUE rb_hash(VALUE);
VALUE rb_hash_new(void);
VALUE rb_hash_dup(VALUE);
VALUE rb_hash_freeze(VALUE);
void rb_hash_modify(VALUE);
VALUE rb_hash_aref(VALUE, VALUE);
VALUE rb_hash_lookup(VALUE, VALUE);
VALUE rb_hash_lookup2(VALUE, VALUE, VALUE);
Expand Down
16 changes: 16 additions & 0 deletions test/ruby/test_hash.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1232,6 +1232,14 @@ def test_update4
assert_equal({1=>8, 2=>4, 3=>4, 5=>7}, h1)
end

def test_update5
h = @cls[a: 1, b: 2, c: 3]
assert_raise(FrozenError) do
h.update({a: 10, b: 20}){ |key, v1, v2| key == :b && h.freeze; v2 }
end
assert_equal(@cls[a: 10, b: 2, c: 3], h)
end

def test_merge
h1 = @cls[1=>2, 3=>4]
h2 = {1=>3, 5=>7}
Expand All @@ -1243,6 +1251,14 @@ def test_merge
assert_equal({1=>8, 2=>4, 3=>4, 5=>7}, h1.merge(h2, h3) {|k, v1, v2| k + v1 + v2 })
end

def test_merge!
h = @cls[a: 1, b: 2, c: 3]
assert_raise(FrozenError) do
h.merge!({a: 10, b: 20}){ |key, v1, v2| key == :b && h.freeze; v2 }
end
assert_equal(@cls[a: 10, b: 2, c: 3], h)
end

def test_assoc
assert_equal([3,4], @cls[1=>2, 3=>4, 5=>6].assoc(3))
assert_nil(@cls[1=>2, 3=>4, 5=>6].assoc(4))
Expand Down