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.

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

Make the dot-colon method reference frozen #2267

Merged
merged 7 commits into from Aug 30, 2019

Conversation

@mensfeld
Copy link
Contributor

@mensfeld mensfeld commented Jul 2, 2019

A follow-up to my discussion with @ko1. This PR freezes the method reference when fetched using the new dot-symbol syntax sugar.

It's worth keeping the bound methods frozen as one should not modify them. Freezing keeps also a window of opportunity for introducing method reference caching in case it would be needed due to the fact, that the method object is immutable.

@mensfeld
Copy link
Contributor Author

@mensfeld mensfeld commented Jul 3, 2019

The appveyor failures are not related to this PR but related to @davydovanton logger changes that were merged before this PR was introduced.

@mensfeld mensfeld force-pushed the frozen-dot-colon-method-reference branch from b22b12b to 5417e7c Jul 3, 2019

def test_method_reference_freeze_state
m = 1.:succ
assert(m.frozen?)
Copy link
Member

@nobu nobu Jul 26, 2019

Choose a reason for hiding this comment

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

assert_predicate will be better for the failure message.

Copy link
Contributor Author

@mensfeld mensfeld Jul 26, 2019

Choose a reason for hiding this comment

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

@nobu done.

Copy link
Member

@nobu nobu Jul 27, 2019

Choose a reason for hiding this comment

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

and assert_not_predicate instead of assert_equal(..., false).

Copy link
Contributor Author

@mensfeld mensfeld Jul 28, 2019

Choose a reason for hiding this comment

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

@nobu fixed again <3

@mensfeld
Copy link
Contributor Author

@mensfeld mensfeld commented Aug 10, 2019

@nobu no rush but if this would be merged, I could open a proposal for the .: last used method cache.

@nobu
Copy link
Member

@nobu nobu commented Aug 11, 2019

Please file at New Issue, and add it to DevelopersMeeting20190829Japan.
Then we'll discuss it at the next meeting.

@mensfeld
Copy link
Contributor Author

@mensfeld mensfeld commented Aug 14, 2019

@nobu https://bugs.ruby-lang.org/issues/16103 - I don't feel competent enough t add it to the developers meeting and me dunno how so I would appreciate if you could drop it there. Thank you in advance!

@mensfeld
Copy link
Contributor Author

@mensfeld mensfeld commented Aug 29, 2019

@nobu I did a rebase, the fails seem not related. I believe, this tests still should be merged.

@nobu
Copy link
Member

@nobu nobu commented Aug 30, 2019

Sorry, I've forgotten this PR.
And a failure in test-bundler should be cleared by rebasing.

@nobu nobu merged commit c45dd4d into ruby:master Aug 30, 2019
10 of 12 checks passed
@mensfeld mensfeld deleted the frozen-dot-colon-method-reference branch Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants