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

vicon_sdk uses C++11 components #11

Closed
wants to merge 2 commits into from
Closed

vicon_sdk uses C++11 components #11

wants to merge 2 commits into from

Conversation

danmox
Copy link
Collaborator

@danmox danmox commented Jul 28, 2018

I added C++11 flags to fix the vicon_sdk compilation error.

@danmox danmox requested a review from kartikmohta July 28, 2018 14:08
@versatran01
Copy link
Collaborator

Prefer

set(CMAKE_CXX_STANDARD 11)

or

set_target_properties(myTarget PROPERTIES
    CXX_STANDARD 11
)

Just my thoughts.

@danmox
Copy link
Collaborator Author

danmox commented Jul 30, 2018

CMAKE_CXX_STANDARD / CXX_STANDARD are available in cmake 3.1+ but the minimum version is set at 2.8.12, hence the use of add_compile_options.

I'm happy to use a more modern approach if you see no issue with bumping the cmake minimum required version to 3.1+.

@mike-watterson-ai
Copy link
Collaborator

The only issue would be with anyone still on 14.04, but I believe most people have upgraded by now.

@tdinesh
Copy link
Collaborator

tdinesh commented Jul 30, 2018

There are some people still on 14.04. Also, the Fetch robot is also still at 14.04. Can you create a branch devel_indigo and then merge this in master? @mdkennedy3

…CXX_STANDARD property to enable C++11 components
@kartikmohta
Copy link
Collaborator

The whole package uses C++11 which was enabled in the main CMakeLists.txt, just that it was done after including the vicon_sdk subdirectory. I just pushed bbc1245 which I think is the simplest fix to this particular issue but we should "modernize" it at some point (probably after 14.04 is EOL).

@kartikmohta
Copy link
Collaborator

@danmox I'll delete the indigo-devel branch and close this PR for now. I created a separate issue (#12) for updating the cmake syntax when we don't need to support Ubuntu 14.04.

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.

5 participants