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

[REQ] Allow compiling with exceptions disabled. #139

Closed
mokafolio opened this issue Jan 4, 2024 · 8 comments
Closed

[REQ] Allow compiling with exceptions disabled. #139

mokafolio opened this issue Jan 4, 2024 · 8 comments
Assignees
Labels
enhancement New feature or request

Comments

@mokafolio
Copy link

Describe the new feature

Allow compiling the threadpool in codebases that have exceptions disabled. Right now that is not possible as far as I can tell (please correct me if I'm wrong). Just from scanning the source, it would only affect the two places try/catch is used.

Additional information

I could look into this and create a PR if that's something you'd consider!

@mokafolio mokafolio added the enhancement New feature or request label Jan 4, 2024
@mokafolio
Copy link
Author

__cpp_exceptions could be used to check if exceptions are enabled or not (https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations)

@bshoshany
Copy link
Owner

Hi @mokafolio and thanks for opening this issue! My design philosophy starting with v4.0.0 has been to implement certain features as optional and let the user decide. I believe your request should be trivial to implement by changing exception handling to an optional feature, and adding a macro (e.g. BS_THREAD_POOL_ENABLE_EXCEPTION_HANDLING) to enable them manually for users who need them. I will incorporate that into the next release (could be either v4.0.2 or v4.1.0 depending how many changes I make by then).

@mokafolio
Copy link
Author

Sounds great! I'd maybe suggest changing the flag to BS_THREAD_POOL_DISABLE_EXCEPTION_HANDLING as I feel like that should be an opt out rather than opt in kind of functionality :)

@bshoshany
Copy link
Owner

Hmm... But all the other macros are BS_THREAD_POOL_ENABLE_xxxxx, and I like to be consistent 😄 Besides, not everyone needs exception handling. In my own projects using this thread pool I never had any situation where I had to catch an exception thrown by a task. And the main reason I like having opt-ins for optional features is that not enabling a feature keeps the library lighter and (potentially) faster.

@mokafolio
Copy link
Author

Yeah, on a personal level I am with you and I also like the consistency. I still think most people expect exception handling to be in place by default but it's your call, I certainly can live with either decision 😄

@mokafolio
Copy link
Author

I still think the least intrusive solution could be to just rely on whether exceptions are enabled or not via __cpp_exceptions . That way it will do the right thing based on the build flags. Adding an extra library level flag does not add that much more usefulness imo.

@bshoshany
Copy link
Owner

Sure, that makes sense. I'll also have to change it so that BS_THREAD_POOL_ENABLE_WAIT_DEADLOCK_CHECK cannot be enabled if exceptions are disabled, or perhaps emits a compiler error/warning. Once I have time to work on a new release I'll look into it.

@bshoshany
Copy link
Owner

Update: This is now implemented in v4.1.0.

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

No branches or pull requests

2 participants