HomeWildfire Games

Clean up ThreadUtil, use standard C++11 constructs instead of custom ones.

Description

Clean up ThreadUtil, use standard C++11 constructs instead of custom ones.

ThreadUtil shipped a scope lock and a mutex implementation, which can be removed since we now have these in the standard library.
This lets us clean up this header which get included everywhere (through profiler includes).

Tested By: Angen and Stan

Differential Revision: https://code.wildfiregames.com/D1915

Event Timeline

Why pthread_t was replaced only for the main thread and not for workers?

/ps/trunk/source/ps/CConsole.h
28
#include <cstdarg>

Why pthread_t was replaced only for the main thread and not for workers?

I don't understand the question, sorry?

I don't understand the question, sorry?

You replaced the:

static pthread_t g_MainThread;

By:

static std::thread::id g_MainThread;

But in some files you didn't replace:

pthread_t m_WorkerThread;
wraitii added a comment.EditedJun 7 2019, 4:11 PM

Ah, ok, then It's probably done in the follow-ups, D1916, D1917 or D1920.

Specifically here I only cleaned up ThreadUtil (and thus had to change a number of files), I didn't yet clean it up elsewhere.

Stan added a subscriber: Stan.Jun 7 2019, 6:06 PM

I probably should have asked that before, but wasn't std::thread broken/missing features on windows ? https://stackoverflow.com/questions/13134186/c11-stdthreads-vs-posix-threads

In rP22344#33832, @Stan wrote:

I probably should have asked that before, but wasn't std::thread broken/missing features on windows ? https://stackoverflow.com/questions/13134186/c11-stdthreads-vs-posix-threads

I think some old supported compilers might not support it.

@wraitii could you check that?

I don't think we're at risk on windows, particularly if we drop VS2013 (soon-ish): https://stackoverflow.com/a/15142193
I also believe we're fine with GCC 4.8.1 and clang 'ought to be unproblematic', so I believe we're in the clear.

elexis raised a concern with this commit.Jul 7 2019, 8:30 PM
elexis added a subscriber: elexis.

Breaks builds without precompiled headers, confirm by reverting this commit and compiling again.

In file included from ../../../source/lib/precompiled.h:71,
                 from ../../../source/pch/engine/precompiled.h:18,
                 from ../../../source/ps/CStrIntern.cpp:18:
../../../source/ps/CStrIntern.cpp: In function ‘CStrInternInternals* GetString(const char*, size_t)’:
../../../source/ps/CStrIntern.cpp:100:9: error: ‘ThreadUtil’ has not been declared
  100 |  ENSURE(ThreadUtil::IsMainThread());
      |         ^~~~~~~~~~
../../../source/lib/debug.h:291:8: note: in definition of macro ‘ENSURE’
  291 |   if(!(expr))\
      |        ^~~~
make[1]: *** [engine.make:276: obj/engine_Release/CStrIntern.o] Error 1
make: *** [Makefile:109: engine] Error 2
make: *** Waiting for unfinished jobs....

rP11422 missed the include, but something (looks like`CLogger.h`, since that was included from CStrIntern.cpp) included the header, so the build only broke after this one.

This commit now has outstanding concerns.Jul 7 2019, 8:30 PM
elexis removed an auditor: elexis.Jul 7 2019, 11:55 PM
This commit no longer requires audit.Jul 7 2019, 11:55 PM

Thanks for taking care of it :)