Skip to content
This repository has been archived by the owner on Feb 7, 2023. It is now read-only.

Require C++11 support with CMake functions. #1027

Conversation

williford
Copy link
Contributor

This PR replaces PR #464. It requires C+11 support using the
new CMake variables (CMAKE_CXX_STANDARD, CMAKE_CXX_STANDARD_REQUIRED,
etc.) when CMake is version 3.1 or above. Otherwise, if CMake is older
(e.g. Ubuntu 14.04) it falls back to using the -std=c++11 flag and
issues a warning.

This PR is based on the comment from @Yangqing:
#464 (comment)

The corresponding line in cmake/MiscCheck.cmake is removed in order to
reduce redundancy. Another option would be to move the C++11 logic to MiscCheck.cmake.

This PR replaces PR facebookarchive#464. It requires C+11 support using the
new CMake variables (CMAKE_CXX_STANDARD, CMAKE_CXX_STANDARD_REQUIRED,
etc.) when CMake is version 3.1 or above. Otherwise, if CMake is older
(e.g. Ubuntu 14.04) it falls back to using the -std=c++11 flag and
issues a warning.

This PR is based on the comment from @Yangqing:
facebookarchive#464 (comment)

The corresponding line in cmake/MiscCheck.cmake is removed in order to
reduce redundancy.
@facebook-github-bot
Copy link
Contributor

@Yangqing has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@Yangqing has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@hgaiser
Copy link
Contributor

hgaiser commented Aug 9, 2017

I still feel this kinda defeats the purpose of using these cmake functions in the first place.. what's the benefit now?

@Yangqing
Copy link
Contributor

Yangqing commented Aug 9, 2017

In fact, this breaks contbuild so I'll revert it...

Yangqing added a commit to Yangqing/caffe2 that referenced this pull request Aug 9, 2017
facebook-github-bot pushed a commit that referenced this pull request Aug 9, 2017
Summary:
TSIA
Closes #1060

Differential Revision: D5590958

Pulled By: Yangqing

fbshipit-source-id: e557eb604e5838255c82c3f59f07f4037cf0a487
@williford
Copy link
Contributor Author

Sorry for the issues with contbuild. I have just signed up for Travis and will check with it next time.

The main benefit from this code is that it throws an error if c++11 is not supported, rather than throwing a more obscure error on c++11 specific code. This could be accomplished by:

  include(CheckCXXCompilerFlag)
  CHECK_CXX_COMPILER_FLAG("-std=c++11" COMPILER_SUPPORTS_CXX11)
  if(NOT COMPILER_SUPPORTS_CXX11)
    MESSAGE(FATAL_ERROR "Your compiler does not support c++11")
  endif()
  set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -std=c11")
  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11")

although this would not work for compilers that do not support -std=c++11 (MSVC?).

I'm guessing requiring this is pretty low priority?

ezyang pushed a commit to ezyang/ATen that referenced this pull request Apr 19, 2018
Summary:
This PR replaces PR #464. It requires C+11 support using the
new CMake variables (`CMAKE_CXX_STANDARD`, `CMAKE_CXX_STANDARD_REQUIRED`,
etc.) when CMake is version 3.1 or above. Otherwise, if CMake is older
(e.g. Ubuntu 14.04) it falls back to using the -std=c++11 flag and
issues a warning.

This PR is based on the comment from Yangqing:
facebookarchive/caffe2#464 (comment)

The corresponding line in cmake/MiscCheck.cmake is removed in order to
reduce redundancy. Another option would be to move the C++11 logic to MiscCheck.cmake.
Closes facebookarchive/caffe2#1027

Differential Revision: D5590646

Pulled By: Yangqing

fbshipit-source-id: 11ac63fbeaab7a1da02115549e214f9c529f1873
zdevito pushed a commit to zdevito/ATen that referenced this pull request Apr 19, 2018
Summary:
This PR replaces PR #464. It requires C+11 support using the
new CMake variables (`CMAKE_CXX_STANDARD`, `CMAKE_CXX_STANDARD_REQUIRED`,
etc.) when CMake is version 3.1 or above. Otherwise, if CMake is older
(e.g. Ubuntu 14.04) it falls back to using the -std=c++11 flag and
issues a warning.

This PR is based on the comment from Yangqing:
facebookarchive/caffe2#464 (comment)

The corresponding line in cmake/MiscCheck.cmake is removed in order to
reduce redundancy. Another option would be to move the C++11 logic to MiscCheck.cmake.
Closes facebookarchive/caffe2#1027

Differential Revision: D5590646

Pulled By: Yangqing

fbshipit-source-id: 11ac63fbeaab7a1da02115549e214f9c529f1873
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants