8000 Fix: Move platform-agnostic files outside of `LIBCORO_FEATURE_THREADING` by bjones1 · Pull Request #175 · jbaldwin/libcoro · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix: Move platform-agnostic files outside of LIBCORO_FEATURE_THREADING #175

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Oct 12, 2023
Merged

Fix: Move platform-agnostic files outside of LIBCORO_FEATURE_THREADING #175

merged 7 commits into from
Oct 12, 2023

Conversation

bjones1
Copy link
Contributor
@bjones1 bjones1 commented Sep 30, 2023

This implements the suggestion in #170.

@bjones1 bjones1 marked this pull request as ready for review September 30, 2023 01:42
jbaldwin
jbaldwin requested changes Oct 6, 2023
View reviewed changes
@bjones1
Copy link
Contributor Author
bjones1 commented Oct 7, 2023

Do you want me to merge in the main branch, or rebase this branch to main? I'm happy to, just let me know your workflow.

Is there anything else to do on this PR?

@jbaldwin
Copy link
Owner
jbaldwin commented Oct 9, 2023

Generally whatever works best, I usually only squash and rebase into main so it's a singular commit in the final git log.

@bjones1
Copy link
Contributor Author
bjones1 commented Oct 10, 2023

Looks like you merged main in successfully -- thanks! Do you need anything else from me on this PR?

I'm excited to get this merged; I think this will get us to successful builds on XCode and MSVC. I'll do some more testing/cleanup after the merge.

jbaldwin
jbaldwin requested changes Oct 12, 2023
View reviewed changes
@jbaldwin
Copy link
Owner

I was planning on merging this tonight, but I did take one last look at it and I do think some of the test files can probably be moved outside of the feature flag pretty easily by switching the tests from coro::io_scheduler to coro::thread_pool. Unless if there is another reason you included them inside the feature flag? Would you mind taking a look and letting me know what you think.

@bjones1
Fix: per conversation in jbaldwin#175, use thread_pool instead of pla…
c65d0fa 
…tform-dependant io_scheduler.
@bjones1
Fix: per conversation in jbaldwin#175, include more tests that don't …
1686ebe 
…depend on LIBCORO_FEATURE_THREADING.
@bjones1
Fix: per conversation in jbaldwin#175, include more tests that don't …
01d24db 
…depend on LIBCORO_FEATURE_THREADING.

However, this makes the test run a LOT slower.
@jbaldwin jbaldwin merged commit cef2ff0 into jbaldwin:main Oct 12, 2023
@jbaldwin jbaldwin mentioned this pull request Oct 12, 2023
Closed
@bjones1 bjones1 deleted the threading-refactor branch October 12, 2023 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbaldwin jbaldwin jbaldwin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bjones1 @jbaldwin
0