Potential fix for 1 code quality finding#3686
Conversation
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
| std::mutex queue_mutex; | ||
| std::condition_variable condition; | ||
| bool stop; | ||
| bool stop = false; |
There was a problem hiding this comment.
After some thought, I've concluded that it is because CThreadPool has two constructors:
CThreadPool() = defaultCThreadPool(size_t)
and only thesize_tversion has a constructor initialisation forstop(false), a few lines further down. The default constructor doesn't, and therefore if that constructor were used,stopwould start uninitialised.
In fact, it would appear that in the code, the CThreadPool constructor is only ever called with the size_t argument, in server.cpp.
So in that case, instead of the above in-class initialisation, I would remove the default constructor declaration and check that the code still compiles (it should, unless I've overlooked something).
There was a problem hiding this comment.
So in that case, instead of the above in-class initialisation, I would remove the default constructor declaration and check that the code still compiles (it should, unless I've overlooked something).
Yes, I've tested compilation with the line CThreadPool() = default; removed, and it still builds fine. So I would recommend that is the best solution, as it doesn't seem to make sense to create a thread pool without specifying a number of threads.
There was a problem hiding this comment.
If one of the two constructors is never needed, removing the unused option makes sense to me.
Also, Gemini mentions Brace Initialization, introduced in C++ v11, which assures zero-initialization.
This PR applies 1/2 suggestions from code quality AI findings. 1 suggestion was skipped to avoid creating conflicts.
Not yet reviewed. We may want to check if this is a valid finding even.