search

bshoshany / thread-pool

BS::thread_pool: a fast, lightweight, and easy-to-use C++17 thread pool library
MIT License
2.21k stars 253 forks source link

[REQ] Allow compiling with exceptions disabled. #139

Closed mokafolio closed 10 months ago

mokafolio commented 10 months ago

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 commented 10 months ago

__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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

Hmm... But all the other macros are BS_THREAD_POOL_ENABLE_xxxxx, and I like to be consistent :smile: 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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 7 months ago

Update: This is now implemented in v4.1.0.