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

Add support for macOS + make architecture dependent section smaller #11

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

juniorrantila
Copy link

This PR adds support for macOS, where the main difference is that all functions have a leading underscore. I also made the target specific assembly section smaller by combining the yield functions in to one. This is possible since the order of the function arguments are the same.

Naked functions are not allowed to contain anything other than asm
blocks. These void casts cause clang to error.
This patch takes advantage of the arguments to the yield functions being
in the same order.
@0dminnimda
Copy link
Contributor

Since you changed the coroutine_switch_context signature, wouldn't linux version stop working?

@juniorrantila
Copy link
Author

Since you changed the coroutine_switch_context signature, wouldn't linux version stop working?

No, it should still work as it did before, since I added an ifdef. I don't have a Linux machine unfortunately, so can't test it, but I'm fairly confident. Feel free to test it though.

@0dminnimda
Copy link
Contributor

I feels like they're too many changes, which don't contribute to adding macos support, like renaming functions, changing their signatures, extracting some operations. Focusing on one chance at a time is much better for pull requests, it's easier to understand for reviewers, it's simpler. So one PR for macos inwhich you literally copy paste things several times, another one for refactoring code, so you don't repeat things so often.

@juniorrantila
Copy link
Author

juniorrantila commented Feb 3, 2025

@0dminnimda Not to be bleak, but in its entirety, this PR is just a couple dozen lines. Though, GitHub diffs can be jarring at times, especially when it comes to macros. If you want to read the code, I encourage you to use the commit view instead, it's usually easier to manage and has the added context of commit messages.

Other than that, this is just something I threw together this morning in a recreational programming session. Tsozin has no obligation to merge any of it nor all of it. He could close the PR with "TLDR" as message for all I care. ¯_(ツ)_/¯

I dare you, double dare you @rexim

@rexim
Copy link
Member

rexim commented Feb 4, 2025

TLDR is the new LGTM /j

I'll look into this a bit later

@0dminnimda
Copy link
Contributor

it would be cool for this to be merged, that's all

@0dminnimda
Copy link
Contributor

I ran it and it actually does work on linux as well, looking through code (not in diff view 😅), I got your idea! It is neat to reuse the first and second argument of yield() as 1 and 2 args of coroutine_switch_context, this way you only have to add the missing third argument, smart!

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

Successfully merging this pull request may close these issues.

3 participants