wjakob / tbb

Intel TBB with CMake build system
Apache License 2.0
369 stars 161 forks source link

CXX standard // switch to target based build flags #50

Closed svenevs closed 4 years ago

svenevs commented 5 years ago

Modifying CMAKE_CXX_FLAGS now that this is on cmake 3.x should be removed in favor of target based build flags. Consider a parent project that is using CMAKE_CXX_STANDARD, a very simple example:

cmake_minimum_required(VERSION 3.3 FATAL_ERROR)
project(supertest)

set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)
add_subdirectory(tbb)

Then when compiling, you will get

$ make VERBOSE=1
...
[  1%] Building CXX object tbb/CMakeFiles/tbb.dir/src/tbb/arena.cpp.o
cd... && /usr/bin/c++ ...  -mrtm -std=c++11... -std=c++14 ... -c /tmp/jaja/tbb/src/tbb/arena.cpp

with -std=c++11 appearing before -std=c++14, which I think (but don't know for sure) that the compiler will typically choose the first one it sees and ignore all others. C++11 and C++14 are mostly compatible but my (not so firm) understanding is that this could be problematic when mixing C++11 and C++17 parent project.

Edit: ref: because of this https://github.com/wjakob/tbb/blob/b066defc0229a1e92d7a200eb3fe0f7e35945d95/CMakeLists.txt#L119

The would-be PR would contain two items:

  1. Use CMAKE_CXX_STANDARD (being careful not to override parent project settings of course!). Note this will not engage the cmake compile-features such as cxx_std_11, CMAKE_CXX_STANDARD is the best way to allow parent projects to control child-cxx-standards, and the goal of this repo is for convenience of add_subdirectory ;)
  2. Replace every instance of set(CMAKE_CXX_FLAGS "... ${CMAKE_CXX_FLAGS}") with target-based compile flags. This would be achieved via an interface library (tbb_interface), but each of tbb, tbb_static, etc, will link privately with tbb_interace so that parent projects do not inherit TBB specific build flags.

Would you like a PR with this?

jschueller commented 4 years ago

I think this could be interesting. The CMAKE_CXX_STANDARD is already set to cxx11.