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

update how c++11 requirement is added #184

Merged
merged 3 commits into from
Feb 20, 2019

Conversation

kejxu
Copy link
Contributor

@kejxu kejxu commented Feb 15, 2019

update how c++11 compiler flag is added:

  • only add it when it's supported (tested on Ubuntu 18.04)
  • add similar logic for MSVC (as mentioned in the comment, this check would fail, but c++11 is always enabled). this pattern could be helpful if later this project moves to c++14 (which is an available switch in MSVC)

the other way to do this is to use target_compile_features(mylib PUBLIC cxx_std_11) from cmake, but this would mean to add this line for every target. would this be preferred instead of adding a global c++11 flag?

kejxu and others added 2 commits February 15, 2019 12:03
* update compiler flag check

* check for c++11 in MSVC and g++
@tfoote
Copy link
Member

tfoote commented Feb 20, 2019

If MSVC is using c++14 by default we shouldn't need to downgrade it for this. Can we just skip the declaration on Windows?

@kejxu
Copy link
Contributor Author

kejxu commented Feb 20, 2019

If MSVC is using c++14 by default we shouldn't need to downgrade it for this. Can we just skip the declaration on Windows?

sure, this was mentioned during our porting too. would we want to keep check_cxx_compiler_flag(-std=c++11 COMPILER_SUPPORTS_CXX11) for G++ or change to just skip this check for MSVC?

@tfoote
Copy link
Member

tfoote commented Feb 20, 2019

We might as well improve the linux logic to check for it before applying it. And then just skip it on MSVC.

@kejxu
Copy link
Contributor Author

kejxu commented Feb 20, 2019

We might as well improve the linux logic to check for it before applying it. And then just skip it on MSVC.

updated to skip /std:c++11 check for msvc and updated comment

@tfoote tfoote merged commit 0d7081c into ros:melodic-devel Feb 20, 2019
@kejxu kejxu deleted the update_cxx11_flag branch February 20, 2019 23:02
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