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

Fix infinite loop #342

Merged
merged 3 commits into from
Oct 12, 2017
Merged

Fix infinite loop #342

merged 3 commits into from
Oct 12, 2017

Conversation

rpavlik
Copy link
Member

@rpavlik rpavlik commented Oct 11, 2017

This was @russell-taylor 's work I just saw on gitk - is this ready to merge? Is it possibly related to OSVR/OSVR-Core#562 ?

…linked was causing infinite loops on Debian 9 Linux.
…braries due to header included in OSVR-Core.
@rpavlik
Copy link
Member Author

rpavlik commented Oct 11, 2017

It would be preferable if we could just do a compiler test for that flag to see if it's supported, rather than just enabling it by force when we see "Linux", FWIW - in theory I know how to do this, given the documentation...

@godbyk
Copy link
Member

godbyk commented Oct 11, 2017

I've used the compiler flag test before:

include(CheckCXXCompilerFlag)
check_cxx_compiler_flag(-fext-numeric-literals SUPPORTS_EXT_NUMERIC_LITERALS)
if(SUPPORTS_EXT_NUMERIC_LITERALS)
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fext-numeric-literals")
endif()

…her than scoping it based on the operating system.
@russell-taylor
Copy link
Contributor

@rpavlik Kevin's fix seems to work on both Windows and Linux and I've added it to the pull request. It could easily be that this fixes the OSVR-Core issue by avoiding overlinking.

Copy link
Contributor

@russell-taylor russell-taylor left a comment

Choose a reason for hiding this comment

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

LGTM with the updated compiler flags.

@russell-taylor russell-taylor merged commit fa2e92d into master Oct 12, 2017
@russell-taylor russell-taylor deleted the fix-infinite-loop branch October 12, 2017 16:03
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.

3 participants