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

Fix compile on win32 #8195

Merged
merged 1 commit into from
Sep 18, 2019

Conversation

straight-shoota
Copy link
Member

This applies the arithmetic operator change to win32-specific implementation and moves references to Fiber behind a flag. The latter can be removed after #7995, but right now it is required to build specs on win32.

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:concurrency topic:stdlib:time labels Sep 17, 2019
Fiber.unsafe_each do |fiber|
fiber.push_gc_roots unless fiber.running?
end
{% unless flag?(:win32) %}
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the reason for this change.
The GC in windows still needs to push the gc root, and the extensions are already behind preview_mt.

Copy link
Member Author

Choose a reason for hiding this comment

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

Master fails to compiler because Fiber is not yet implemented on win32. It is only added in #7995

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only temporary and will be removed in #7995. But it's necessary to run the specs prior to that.

# (IO stuff, sleep, etc.). Without it the user might wait more than needed
# after pressing CTRL+C to quit the tests.
Fiber.yield
{% unless flag?(:win32) %}
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is because Fiber.yield is not allowed in windows. If so, then maybe is better to add the flag?(:win32) check inside Fiber.yield

Copy link
Member Author

Choose a reason for hiding this comment

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

Fiber itself is currently undefined in win32.

Copy link
Member

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

Perfect

@RX14 RX14 added this to the 0.31.0 milestone Sep 18, 2019
@RX14 RX14 merged commit 319efb3 into crystal-lang:master Sep 18, 2019
@straight-shoota straight-shoota deleted the fix/win32-crystal-0.30.1 branch September 18, 2019 22:33
dnamsons pushed a commit to dnamsons/crystal that referenced this pull request Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:concurrency topic:stdlib:time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants