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.

Sign up for GitHub

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

cmake: Check for C++-17 and conditionally enable it #188

Closed
wants to merge 1 commit into from
Closed

cmake: Check for C++-17 and conditionally enable it #188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,15 @@ set(CMAKE_CXX_STANDARD 14)
set(CMAKE_POSITION_INDEPENDENT_CODE ON)
set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")

include(CheckCXXCompilerFlag)

if("${CMAKE_MAJOR_VERSION}" EQUAL 3 AND "${CMAKE_MINOR_VERSION}" GREATER 7)
check_cxx_compiler_flag(-std=c++17 HAVE_FLAG_STD_CXX17)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best way to do this that I've found is to search CMAKE_CXX_COMPILE_FEATURES for cxx_std_17:

list(FIND CMAKE_CXX_COMPILE_FEATURES cxx_std_17 cxx17_index)
if(NOT ${cxx17_index} EQUAL -1)
    set(CMAKE_CXX_STANDARD 17)
endif()

This currently will not behave as desired on MSVC.

This change could be rebased on top of #192 - it's no longer needed to make it build on gcc 9.3.0 (I hope!) but it would still be worth considering an "opportunistic" upgrade to C++17.

if(HAVE_FLAG_STD_CXX17)
set(CMAKE_CXX_STANDARD 17)
endif()
endif()

include(GNUInstallDirs)

### Dependencies
Expand Down
18 changes: 13 additions & 5 deletions src/loader/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,19 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Linux")
endif()

set_target_properties(openxr_loader PROPERTIES SOVERSION "${MAJOR}" VERSION "${MAJOR}.${MINOR}.${PATCH}")
target_link_libraries(
openxr_loader
PRIVATE stdc++fs
PUBLIC m
)

if(HAVE_FLAG_STD_CXX17)
target_link_libraries(
openxr_loader
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this could be split into two target_link_libraries calls - one with just m, and one with stdc++fs. However, m isn't apparently actually needed, and at least on my gcc 8.3.0, I need stdc++fs even though I have c++17 support. I'd suggest just dropping this part of the patch when rebasing on #192 since that handles it in a more complete way.

PUBLIC m
)
else()
target_link_libraries(
openxr_loader
PRIVATE stdc++fs
PUBLIC m
)
endif()

add_custom_target(
libopenxr_loader.so.${MAJOR}.${MINOR} ALL
Expand Down