Skip to content
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

Replaced platform-specific mutex implementations with std::recursive_… #3425

Merged
merged 1 commit into from
Nov 3, 2022

Conversation

seanm
Copy link
Contributor

@seanm seanm commented Nov 2, 2022

…mutex

This fixes a clang -Wreserved-id-macro warning by no longer ever needing to #define _GNU_SOURCE.

@rouault
Copy link
Member

rouault commented Nov 2, 2022

The following snippet in src/lib_proj.cmake

option(USE_THREAD "Build libproj with thread/mutex support " ON)
if(NOT USE_THREAD)
  add_definitions(-DMUTEX_stub)
endif()
find_package(Threads QUIET)
if(USE_THREAD AND Threads_FOUND AND CMAKE_USE_WIN32_THREADS_INIT)
  add_definitions(-DMUTEX_win32)
elseif(USE_THREAD AND Threads_FOUND AND CMAKE_USE_PTHREADS_INIT)
  add_definitions(-DMUTEX_pthread)
elseif(USE_THREAD AND NOT Threads_FOUND)
  message(FATAL_ERROR
    "No thread library found and thread/mutex support is "
    "required by USE_THREAD option")
endif()

should also be reworked to something like:

find_package(Threads QUIET)
if(NOT Threads_FOUND)
  message(FATAL_ERROR  "No thread library found")
endif()

and include/proj/internal/mutex.hpp should be removed, and its users modified to use std::mutex

@seanm
Copy link
Contributor Author

seanm commented Nov 2, 2022

and include/proj/internal/mutex.hpp should be removed, and its users modified to use std::mutex

So the comment about "mingw32-win32 doesn't implement std::mutex" is no longer a concern?

In the meantime, I've updated the CMake files...

@rouault
Copy link
Member

rouault commented Nov 2, 2022

So the comment about "mingw32-win32 doesn't implement std::mutex" is no longer a concern?

I was wondering, but we have no CI config testing that, and I'm not sure we should care abou a platform not implementing std::mutex

@seanm
Copy link
Contributor Author

seanm commented Nov 2, 2022

I was wondering, but we have no CI config testing that, and I'm not sure we should care abou a platform not implementing std::mutex

Well, as you require C++11 already, you are essentially already requiring std::mutex :)

…mutex

Simplified various CMake files too, though they still need to look for pthread support because there is a call to pthread_atfork() in the codebase.

Also got rid of mutex.hpp and its workaround for old __MINGW32__ that did not support std:mutex.

Aside from being a general simplification, this fixes a clang -Wreserved-id-macro warning by no longer ever needing to #define _GNU_SOURCE.
@rouault rouault added this to the 9.2.0 milestone Nov 3, 2022
@rouault rouault merged commit 08f691c into OSGeo:master Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants