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

Get rid off dependency on Signal in Win32 Process::Status #8537

Conversation

jan-zajic
Copy link
Contributor

Remove dependency on Signal on win32 platform.

@jan-zajic jan-zajic mentioned this pull request Nov 29, 2019
@jan-zajic jan-zajic force-pushed the feature/win32_remove_signal_dep branch from c704eb4 to 4a701dc Compare November 29, 2019 19:21
src/compiler/crystal/compiler.cr Outdated Show resolved Hide resolved
src/compiler/crystal/tools/playground/server.cr Outdated Show resolved Hide resolved
@jan-zajic jan-zajic force-pushed the feature/win32_remove_signal_dep branch from 4a701dc to ebc2457 Compare November 30, 2019 20:21
@straight-shoota
Copy link
Member

I'd like to see a comment for each location that is removed which explains why this doesn't work for win32, whether that's a problem and if this needs attention later.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

Maybe we can have a minimal Signal enum on Win32 (with a TODO: implement signals on Win32), then make Process::Status.exit_signal return a dummy Process::Status.new(-1). That would allow to keep the status.exit_signal usages —or wrap/guard them with if status.signal_exit?.

Signal exit is only useful on POSIX, but a dummy is harmless and transparent on Win32, so developers don't have to deal with platform discrepancies.

@RX14
Copy link
Contributor

RX14 commented Dec 5, 2019

Yeah, I'd much rather add something to the windows stubs file than ifdef out code. Ifdefs should be kept to a minimum.

@straight-shoota
Copy link
Member

The result of #8647 is also relevant for this.

@straight-shoota straight-shoota added breaking-change kind:refactor platform:windows Windows support based on the MSVC toolchain / Win32 API pr:needs-work A PR requires modifications by the author. topic:stdlib:system labels Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change kind:refactor platform:windows Windows support based on the MSVC toolchain / Win32 API pr:needs-work A PR requires modifications by the author. topic:stdlib:system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants