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

Deprecate Channel.select splat overloads #12813

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

straight-shoota
Copy link
Member

The splat-receiving overloads of Channel.select and Channel.blocking_select are internal methods (:nodoc:) but never used internally. They can be removed. This patch only adds a deprecation, but it would probably be fine to just drop them entirely.

They used to be part of the public select API. After the introduction of the select keyword in #3130 they remained visible, but became nodoc in #9564.

@bcardiff
Copy link
Member

bcardiff commented Dec 8, 2022

I thought the compiler will emit call to the splat overloads. But the expanded calls use Channel.non_blocking_select({ch.send_select_action}) instead of Channel.non_blocking_select(ch.send_select_action) as I thought.

I would prefer to this splat overloads to remain. So the compiler could emit the later and we can add overloads for optimized version for single/double channel actions.

Of course we can check on the indexable size on runtime, so it's not big deal if we deprecate them.

@straight-shoota straight-shoota self-assigned this Dec 8, 2022
Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @straight-shoota 🙏

@straight-shoota straight-shoota modified the milestone: 1.7.0 Dec 22, 2022
@straight-shoota straight-shoota marked this pull request as draft January 10, 2023 18:41
@straight-shoota
Copy link
Member Author

Draft for now because it's not clear whether we might want to keep using these overloads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants