-
Notifications
You must be signed in to change notification settings - Fork 612
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix macOS and Windows build failures when threading is disabled #1462
Fix macOS and Windows build failures when threading is disabled #1462
Conversation
Addresses AcademySoftwareFoundation#1455 The logic around which implementation of the IlmThread::Semaphore class to use is now consolidated into the block of code that deciphers the platform and specifies the #includes, rather than spread across multiple files. Signed-off-by: Cary Phillips <[email protected]>
Signed-off-by: Cary Phillips <[email protected]>
Signed-off-by: Cary Phillips <[email protected]>
Signed-off-by: Cary Phillips <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks correct, and also, less tangled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. C++20 as a minimum required c++ version with counting_semaphore (among other things) can not come too soon...
…emySoftwareFoundation#1462) * Fix macOS and Windows build failures when threading is disabled Addresses AcademySoftwareFoundation#1455 The logic around which implementation of the IlmThread::Semaphore class to use is now consolidated into the block of code that deciphers the platform and specifies the #includes, rather than spread across multiple files. Signed-off-by: Cary Phillips <[email protected]> * Add macOS/Windows builds with threading disabled Signed-off-by: Cary Phillips <[email protected]> * Fix typo with ILMTHREAD_SEMAPHORE_OTHER, and add missing macOS includes Signed-off-by: Cary Phillips <[email protected]> * Add threads-enabled='ON' so it shows up in the job name Signed-off-by: Cary Phillips <[email protected]> --------- Signed-off-by: Cary Phillips <[email protected]>
* Fix macOS and Windows build failures when threading is disabled Addresses #1455 The logic around which implementation of the IlmThread::Semaphore class to use is now consolidated into the block of code that deciphers the platform and specifies the #includes, rather than spread across multiple files. Signed-off-by: Cary Phillips <[email protected]> * Add macOS/Windows builds with threading disabled Signed-off-by: Cary Phillips <[email protected]> * Fix typo with ILMTHREAD_SEMAPHORE_OTHER, and add missing macOS includes Signed-off-by: Cary Phillips <[email protected]> * Add threads-enabled='ON' so it shows up in the job name Signed-off-by: Cary Phillips <[email protected]> --------- Signed-off-by: Cary Phillips <[email protected]>
Addresses #1455
The logic around which implementation of the
IlmThread::Semaphore
class to use is now consolidated into the block of code that deciphers the platform and specifies the #includes, rather than spread across multiple files.This also adds macOS and Windows CI jobs with threading disabled.
The previous logic was completely broken for the macOS case, where it would produce duplicate implementations in
IlmThreadSemaphore.cpp
and inIlmThreadSemaphoreOSX.cpp
when threading is disabled.Please do what you can to confirm that this logic looks correct for all platforms, since it's not completely clear to me that the test suite will reliably fail if the logic is off.