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

kernel: Refactor after_fork_child_callbacks #4692

Merged

Conversation

bew
Copy link
Contributor

@bew bew commented Jul 9, 2017

The nils were probably needed, when the type inference wasn't powerful enough. They are not necessary anymore.

@bew bew mentioned this pull request Jul 9, 2017
src/kernel.cr Outdated
->{ Event::SignalHandler.after_fork; nil },
->{ Event::SignalChildHandler.instance.after_fork; nil },
->{ Random::DEFAULT.new_seed; nil },
->{ Scheduler.after_fork },
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to use just ->Scheduler.after_fork syntax here.

Copy link
Contributor Author

@bew bew Jul 9, 2017

Choose a reason for hiding this comment

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

True, it works for most calls, I changed it!

It doesn't work for ->Event::SignalChildHandler.instance.after_fork because there is 2 calls, first instance then after_fork, we can't shorten this one.

@bew bew force-pushed the refactor-kernel-after-fork-callbacks branch from a4ea3b8 to 33fce60 Compare July 9, 2017 21:03
@bew bew changed the title kernel: Refactor after_fork_child_callbacks, remove unnecessary nils kernel: Refactor after_fork_child_callbacks Jul 9, 2017
@mverzilli mverzilli merged commit d803e78 into crystal-lang:master Jul 9, 2017
@mverzilli
Copy link

Nice catch, thank you!

@mverzilli mverzilli added this to the Next milestone Jul 9, 2017
@bew bew deleted the refactor-kernel-after-fork-callbacks branch July 9, 2017 22:36
@bew bew mentioned this pull request Sep 1, 2017
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