progschj / ThreadPool

A simple C++11 Thread Pool implementation
zlib License
7.64k stars 2.21k forks source link

Semantic/format and queue emplace improvement. #18

Closed yxbh closed 9 years ago

yxbh commented 9 years ago

I'm purposing a semantic/format fix to fix some inconsistency where some parts of the code would refer to the members via "this->" and others reference the member variables directly. I've changed all to "this->mem" format. Some brackets are moved to be more consistent over all. I've also added void to signatures for readability. I realise this is just personally preference and you might not like it, so it's okay to not accept this.

I've also changed push to emplace in the enqueue method. Since we are constructing a std::function with a lambda, I think emplace makes sense here.

I also think it would be appropriate to have a default constructor as below:

ThreadPool(void) : ThreadPool(std::thread::hardware_concurrency() > 0 ? std::thread::hardware_concurrency() : 2) {}

It would look better if we have a static local function

static size_t getDefaultThreadPoolSize(void) { const auto size = std::thread::hardware_concurrency(); return size > 0 ? size : 2; // or another suitable number. }

then we can have:

ThreadPool(void) : ThreadPool(getDefaultThreadPoolSize()) {}

Let me know what you think.

yxbh commented 9 years ago

Alright, fair enough :P

Should I close the pull request then?

Darelbi commented 9 years ago

I don't think is a good Idea using hardware concurrency as default value for poolsize. The reason is that "Default Pool Size" can be assumed constant by someone using the library while instead its value will change depending on the device in wich code run.. Or Am I just too pessimistic?