Return to Answer

Also refer to this question.

While not technically wrong in the context you've used, names starting with an underscore bare a few restrictions in C++. FYI, I suggest reading this SO question.

Also refer to this question.

While not technically wrong in the context you've used, names starting with an underscore bare a few restrictions in C++. FYI, I suggest reading this SO question.

To have a forward declaration of those types and avoid the #includes. I don't like this though. There is no guarantee that mutex or condition_variable are plain class types. They could be anything, structs, template classes, or even typedefs to anything else. so this is certainly not portable across compilers and might even break on a future update of your current compiler. Including the relevant files in the header is cleaner, safer and also simpler.

The busy wait loop in Worker::Join() could take advantage of std::this_thread::yield(), which might perform better than just spinning:

Those comments at the end of every function are plain noise.

As it stands, your classes can be copied using operator = or the implicit copy constructor. Doing so would wreak havoc in your program, since multiple attempts to delete the same pointers would be done. You should delete the copy constructor and assignment operator of Semaphore and Worker, and perhaps also from Thread. E.g.:

This falls into what is known as Rule of Three, which means that if a construtor does some non-trivial work, such as memory allocation, then more than likely you'll need to define custom copy constructor and assignment as well.

In the Semaphore, condition_variable and mutex are declared as pointers. I would declare them by value to avoid extra memory allocations and improve data locality. I don't see any obvious reason for them to be pointers. If you do keep the pointers and dynamic allocations, then consider using a std::unique_ptr rather than manual new and delete calls, to ensure exception safety and simplify the resource management logic. Same applies to thread and mutex in Worker. Those could be declared by value as well. Otherwise, would be better as smart pointers too.

To have a forward declaration of those types and avoid the #includes. I don't like this though. There is no guarantee that mutex or condition_variable are plain class types. They could be anything, structs, template classes, or even typedefs to anything else. So this is certainly not portable across compilers and might even break on a future update of your current compiler. Including the relevant files in the header is cleaner, safer and also simpler.

Also refer to this question.

The busy wait loop in Worker::Join() could take advantage of std::this_thread::yield(), which might perform better than just spinning.

Those comments at the end of every other method are plain noise.

As it stands, your classes can be copied using operator = or the implicit copy constructor. Doing so would wreak havoc in your program, since each copy would be a shallow copy and multiple attempts to delete the same pointers would be done by the destructors of different objects. You should at the very least delete the copy constructor and assignment operator of Semaphore and Worker, and perhaps also from Thread. E.g.:

This falls into what is known as Rule of Three, which means that if a construtor does some non-trivial work, such as memory allocation, then more than likely you'll need to define custom copy constructor and assignment as well (or disable them).

In the Semaphore class, condition_variable and mutex are declared as pointers. I would declare them by value to avoid extra memory allocations and improve data locality. I don't see any obvious reason for them to be pointers. If you do keep the pointers and dynamic allocations, then consider using a std::unique_ptr rather than manual new and delete calls, to ensure exception safety and simplify the resource management logic. Same applies to thread and mutex in Worker. Those could be declared by value as well. Otherwise, would be better as smart pointers too.

I imagine you did this:

namespace std
{
 class mutex;
 class condition_variable;
}//namespace std

To have a forward declaration of those types and avoid the #includes. I don't like this though. There is no guarantee that mutex or condition_variable are plain class types. They could be anything, structs, template classes, or even typedefs to anything else. so this is certainly not portable across compilers and might even break on a future update of your current compiler. Including the relevant files in the header is cleaner, safer and also simpler.

The busy wait loop in Worker::Join() could take advantage of std::this_thread::yield(), which might perform better than just spinning:

void Worker::Join() const
{ 
 while (_tasks.size())
 {
 std::this_thread::yield();
 }
}

Those comments at the end of every function are plain noise.

void ThreadPool::ClearTasks()
{
 for (auto& worker : _workers)
 worker.ClearTasks();
}//ClearTasks

Your methods are not that big, I can see where they begin and end very easily without that comment.

While not technically wrong in the context you've used, names starting with an underscore bare a few restrictions in C++. FYI, I suggest reading this SO question.

Memory Management:

In the Semaphore, condition_variable and mutex are declared as pointers. I would declare them by value to avoid extra memory allocations and improve data locality. I don't see any obvious reason for them to be pointers. If you do keep the pointers and dynamic allocations, then consider using a std::unique_ptr rather than manual new and delete calls, to ensure exception safety and simplify the resource management logic. Same applies to thread and mutex in Worker. Those could be declared by value as well. Otherwise, would be better as smart pointers too.

Miscellaneous:

I imagine you did this:

namespace std
{
 class mutex;
 class condition_variable;
}//namespace std

To have a forward declaration of those types and avoid the #includes. I don't like this though. There is no guarantee that mutex or condition_variable are plain class types. They could be anything, structs, template classes, or even typedefs to anything else. so this is certainly not portable across compilers and might even break on a future update of your current compiler. Including the relevant files in the header is cleaner, safer and also simpler.

The busy wait loop in Worker::Join() could take advantage of std::this_thread::yield(), which might perform better than just spinning:

void Worker::Join() const
{ 
 while (_tasks.size())
 {
 std::this_thread::yield();
 }
}

Those comments at the end of every function are plain noise.

void ThreadPool::ClearTasks()
{
 for (auto& worker : _workers)
 worker.ClearTasks();
}//ClearTasks

Your methods are not that big, I can see where they begin and end very easily without that comment.

While not technically wrong in the context you've used, names starting with an underscore bare a few restrictions in C++. FYI, I suggest reading this SO question.

Copy and assignment:

As it stands, your classes can be copied using operator = or the implicit copy constructor. Doing so would wreak havoc in your program, since multiple attempts to delete the same pointers would be done. You should delete the copy constructor and assignment operator of Semaphore and Worker, and perhaps also from Thread. E.g.:

class Semaphore final
{
 Semaphore(const Semaphore &) = delete;
 Semaphore & operator = (const Semaphore &) = delete;
 // copy and assignment are now disallowed.
};

This falls into what is known as Rule of Three , which means that if a construtor does some non-trivial work, such as memory allocation, then more than likely you'll need to define custom copy constructor and assignment as well.

Memory Management:

In the Semaphore, condition_variable and mutex are declared as pointers. I would declare them by value to avoid extra memory allocations and improve data locality. I don't see any obvious reason for them to be pointers. If you do keep the pointers and dynamic allocations, then consider using a std::unique_ptr rather than manual new and delete calls, to ensure exception safety and simplify the resource management logic. Same applies to thread and mutex in Worker. Those could be declared by value as well. Otherwise, would be better as smart pointers too.