fixes MSVC (MSVC2019, ...) errors when projects are used which have _CRT_SECURE_NO_WARNINGS already set as part of the default build settings

[GerHobbelt]

fixes for MSVC (MSVC2019, ...) and when projects are used which have _CRT_SECURE_NO_WARNINGS already set as part of the default build settings.

---

Testing

This was tested as part of a larger work (other PRs are forthcoming shortly) after hunting down shutdown issues (application lockups, etc.) in a large application.

Tested this code via your provided test code rig; see my own fork and the referenced commits which point into there.

Tested on AMD Ryzen 3700X, 128GB RAM, latest Win10/64, latest MSVC2019 dev environment. Using in-house project files which use a (in-house) standardized set of optimizations.

Additional information

TBD

The patches are hopefully largely self-explanatory. Where deemed useful, the original commit messages from the dev fork have been referenced and included.

[bshoshany]

Thanks for this pull request! This is a good idea.

In the next release, I plan to stop using std::localtime, and this should eliminate the need to define _CRT_SECURE_NO_WARNINGS altogether. However, if I find that reproducing the results of std::localtime in a thread-safe way (while still only using standard C++17) requires too much effort, I will incorporate your proposed change into the code.

Also, may I ask why you're adding #undef min and #undef max?

[GerHobbelt]

Sure.

Collision between std::min and std::max and min and max defines in windows.h (or one of the header files it pulls in), when I use the threadpool in another codebase, where those #includes have already been loaded before the thread-pool code is #included.

https://stackoverflow.com/questions/5004858/why-is-stdmin-failing-when-windows-h-is-included#answer-5004874, but NOMINMAX was, shall we say, not an option either. 😢 Yeah.

[bshoshany]

Interesting... I never use Windows.h, so I never encountered this issue before. But I just checked and found that changing const int64_t offset = std::min(random_start, static_cast<int64_t>(random_end)); to const int64_t offset = std::min<int64_t>(random_start, static_cast<int64_t>(random_end)); also resolves this issue, while still being 100% portable and not adding any extra lines, so I think that's what I'll do instead.

[GerHobbelt]

👍

[bshoshany]

Update: the solution for the collision with Windows.h is now implemented in v3.4.0. The collision with _CRT_SECURE_NO_WARNINGS is something I'm still working on and should be implemented (one way or another) in the next release.

[bshoshany]

Update: I spent some time looking into portable thread-safe alternatives to std::localtime and could not find any (std::chrono was expanded for this purpose in C++20, but this project is C++17-compatible, so I cannot use that). So I will just incorporate your solution of only setting _CRT_SECURE_NO_WARNINGS if it is not already defined. This will be implemented in the upcoming v3.5.0 release.

[bshoshany]

Update: This issue has been fixed in v3.5.0, which was just released. Thanks again!