C++11 std mutex and condition variable #1298
Conversation
|
|
||
|
|
||
|
|
||
| Dynamic __hxcpp_mutex_create() |
There was a problem hiding this comment.
Could we omit these for hxcpp_api_level >= 500? It would be good to deprecate them anyway since identifiers containing __ are reserved by c++.
There was a problem hiding this comment.
I will do this but in a separate PR, other wise I think we end up in a situation where the CIs need two sets of changes merged at the same time to go green.
| } | ||
|
|
||
|
|
||
| HxMutex mMutex; |
There was a problem hiding this comment.
Since the mutex no longer uses HxMutex, I think we should also remove the HxMutex type from everywhere else in the codebase for internal consistency.
That also officially breaks the HX_THREAD_H_OVERRIDE feature, so we would have to remove that too, or continue to have a HxMutex wrapper.
There was a problem hiding this comment.
I think replacing uses of HxMutex with std::recursive_mutex makes sense. As a replacement for HX_THREAD_H_OVERRIDE maybe have a hxml & xml HX_THREAD_API_XML define which you can point to a custom xml file which will provide the implementation of the ConditionVariable and RecursiveMutex types?
There was a problem hiding this comment.
Seems reasonable, though if it is for hxml use it should be prefixed with HXCPP_ for consistency.
There was a problem hiding this comment.
There's only one use of HxMutex / a bit of manual pthread condition left in Immix.cpp now, unfortunately this last one is a bit of a pain. C++ states that when the dtor of std::condition_variable gets called no thread should have it locked, however on exit hxcpp programs hang when adapting the last uses of pthread conditions to std::condition_variable as some thread seems to be holding onto the locks.
There was a problem hiding this comment.
I've solved that, the only remaining use of HxMutex is in the Deque and Lock implementation. These two also seem a bit tricky as there is differing implementations for Windows and non Windows platforms (not sure why).
Think I'll also try and get those sorted in a different PR.
| return cond->TimedWait(timeout); | ||
| bool __hxcpp_condition_timed_wait(Dynamic inCond, double timeout) | ||
| { | ||
| return hx::Throw(HX_CSTRING("Not Implemented")); |
There was a problem hiding this comment.
If this is being removed, I think it would be better to remove it completely rather than keeping it and having a runtime error.
There was a problem hiding this comment.
My thinking was I'd get rid of it along with the other functions after it's merged in so the CI on the haxe side can stay green with the pending PR.
|
Let me know when this is good to merge, I'd like to get hxcoro back to hxcpp master soon to see if we benefit from the pch change! |
|
I think it's good to go, the remaining things mentioned are best done in a separate PR after the haxe side is merged so we can keep the CIs green. |
|
Alright! Any change to Immix.cpp terrifies me, but if something comes up I'm sure we (= you) will figure it out! |
4 participants
Replaces the old implementation which had a bunch of ifdefs depending on the platform. Now we have a single implementation using the C++
stdtypes. I would have done the same forLockbut for some reason it took until C++20 to get a semaphore type...I have broken the definitions out into their own headers and exposed the classes so on the haxe side we can remove the untyped code.