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

wants to merge 2 commits into from

Conversation

kachick
Copy link
Member

@kachick kachick commented Mar 20, 2021

This PR aims a part of https://bugs.ruby-lang.org/issues/17736

$ ruby -w -v -e '(h={a: 1, b: 2, c: 3}).update({b: 4}) { h.freeze; 42 }; p h'
ruby 3.0.0p0 (2020-12-25 revision 95aff21468) [x86_64-darwin20]
{:a=>1, :b=>42, :c=>3}

nobu added a commit to nobu/ruby that referenced this pull request Mar 21, 2021
@nobu
Ensure the receiver modifiable before updating [Bug #17736]
f006ffc 
Close ruby#4298
nobu
nobu reviewed Mar 21, 2021
View reviewed changes
hash.c
@@ -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! 🙏

hash.c
@@ -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 🙇

@matzbot matzbot closed this in d36ac28 Mar 21, 2021
@kachick
Copy link
Member Author

kachick commented Mar 21, 2021

Thank you for merging adding test commit and making more considerate patch to fix the issue 🙏

@kachick kachick deleted the bug/17736-hash-update-ensure-modifiable branch March 21, 2021 15:54
@kachick kachick mentioned this pull request May 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nobu nobu nobu left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
2 participants
@kachick @nobu