8000 Proposed changes to threading option · Issue #170 · jbaldwin/libcoro · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Proposed changes to threading option #170

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
Open
bjones1 opened this issue Sep 20, 2023 · 4 comments
Open

Proposed changes to threading option #170

bjones1 opened this issue Sep 20, 2023 · 4 comments

Comments

@bjones1
Copy link
Contributor
bjones1 commented Sep 20, 2023

Since I'm working on getting libcoro ported to other compilers/platforms, would you be open to a redefinition of the LIBCORO_FEATURE_THREADING option in CMakeLists.txt? Current, the docstring there is "Include multithreading features." Could we make this "Include platform-specific multithreading features", and include all features that are platform-independent for all platforms? Almost all of the current threading features will work on Windows (and probably all platforms).
@jbaldwin
Copy link
Owner

Would this require a LIBCORO_FEATURE_THREADING_WINDOWS|LINUX cmake option if I'm understanding correctly?

What about detecting the compiler/platform and including the appropriate features that way and keeping the option named the same? That way the user doesn't have to care about which platform they are on for the option, if its enabled it'll include as much as possible that that platform supports. Might be good to document what features each platform would include in the readme

@bjones1
Copy link
Contributor Author
bjones1 commented Sep 28, 2023

Would this require a LIBCORO_FEATURE_THREADING_WINDOWS|LINUX cmake option if I'm understanding correctly?

Instead, I was thinking of making the LIBCORO_FEATURE_THREADING include Linux-only features, and including everything that's platform-agnostic by default.

What about detecting the compiler/platform and including the appropriate features that way and keeping the option named the same? That way the user doesn't have to care about which platform they are on for the option, if its enabled it'll include as much as possible that that platform supports. Might be good to document what features each platform would include in the readme

I agree that we should automatically exclude features that aren't supported on the OS used to build the library. But I also think that most of the libcoro features should be included by default; after all, as a library, developers can simply link against what they want; there's little penalty for including more. In addition, a large amount of libcoro is implemented as header files, so there's no executable code to include in a library.

Here's a concrete proposal for making most of the LIBCORO_FEATURE_THREADING an unconditional part of the build, leaving only Linux-specific files behind that feature. This could be improved by future PRs supporting other OSes.

Files that are currently in LIBCORO_FEATURE_THREADING that would be an unconditional part of the build:

  • include/coro/concepts/awaitable.hpp

  • include/coro/concepts/buffer.hpp

  • include/coro/concepts/executor.hpp

  • include/coro/concepts/promise.hpp

  • include/coro/concepts/range_of.hpp

  • include/coro/detail/void_value.hpp

  • include/coro/coro.hpp

  • include/coro/event.hpp src/event.cpp

  • include/coro/generator.hpp

  • include/coro/latch.hpp

  • include/coro/mutex.hpp src/mutex.cpp

  • include/coro/ring_buffer.hpp

  • include/coro/semaphore.hpp src/semaphore.cpp

  • include/coro/shared_mutex.hpp

  • include/coro/sync_wait.hpp src/sync_wait.cpp

  • include/coro/task.hpp

  • include/coro/task_container.hpp

  • include/coro/thread_pool.hpp src/thread_pool.cpp

  • include/coro/when_all.hpp

@jbaldwin
Copy link
Owner

I think this makes a lot of sense now that you broken it down, I like it a lot.

I will note thought that the original ask behind LIBCORO_FEATURE_THREADING was for an embedded platform without threads, so its not technically linux vs windows... perhaps what you are suggesting is more LIBCORO_FEATURE_PLATFORM_COMMON? I'm not exactly sure what a good name for it is or if I'm even thinking down the right track on this. Or maybe THREADING/COMMON are basically an overlap so it works for both?

I think what really should happen is abstracting out the linux parts behind an interface that any platform could then implement properly. This is probably a step in that direction? So I think its a good thing.

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

I think this makes a lot of sense now that you broken it down, I like it a lot.

Fantastic!

I will note thought that the original ask behind LIBCORO_FEATURE_THREADING was for an embedded platform without threads, so its not technically linux vs windows... perhaps what you are suggesting is more LIBCORO_FEATURE_PLATFORM_COMMON? I'm not exactly sure what a good name for it is or if I'm even thinking down the right track on this. Or maybe THREADING/COMMON are basically an overlap so it works for both?

That makes sense -- offer a flavor of the library that's platform agnostic, so it runs on embedded platforms or any OS. I'd suggest this part of the code. Given that, why place it behind a flag at all? I'd instead suggest enabling that part of the code for all builds.

I think what really should happen is abstracting out the linux parts behind an interface that any platform could then implement properly. This is probably a step in that direction? So I think its a good thing.

That makes sense! I use the Qt library; perhaps we could borrow a bit of inspiration from their approach, since they've created nice abstractions around a lot of OS-specific stuff.

@bjones1 bjones1 mentioned this issue Sep 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jbaldwin @bjones1
0