[cmake-developers] Please review CXXFeatures.cmake

[Stephen Kelly]

Rolf Eike Beer wrote:

> Am Sonntag, 28. April 2013, 13:57:26 schrieb Rolf Eike Beer:
>> One question I see increasingly often is "how do I test for C++11
>> support" or for specific parts of that. For 2.8.12 I plan to include the
>> check module I wrote for that a while back, and that I have reworked in
>> the last weeks. You can find the current state in the "rework" branch of
>> this repository:
> 
> Ok, I finally put it into the CMake repository:
> 

I have some feedback:

1) 

A recent commit in the topic does this:

 -g++ seems to support constexpr since 4.5 even if their support page says 
4.6

It has been common for compilers to implement support for features like 
this, but have some fundamental brokenness in earlier versions. Your test 
which enables the use of constexpr might not exercise the feature enough to 
hit the fundamental brokenness, but that doesn't mean it's an edge-case. It 
would be better not to second-guess the version gcc says it supports 
features in.

2)

Given that you're gathering the versions of each feature availability 
anyway, and given that boost.config and qcompilerdetection.h have the same 
information, there is no need for all users of the module to run all these 
try_compiles for all projects. Think of the energy waste :)!

I suggest you use CMAKE_CXX_COMPILER_ID and CMAKE_CXX_COMPILER_VERSION to 
hardcode the features. You could even do so for known compilers, and leave 
the try_compile stuff for not-known compilers if you really want to, but I 
don't think that's worthwhile maintenance.

3)

CXX11_COMPILER_FLAGS is not really the 'modern' way to specify compiler 
flags, as of recent CMake versions. See for example the reasoning here:

 http://thread.gmane.org/gmane.comp.programming.tools.cmake.devel/6876/focus=6890

regarding *_FLAGS, which is a whitespace delimited string, and 
COMPILE_OPTIONS, which is a regular ';' delimited CMake list.

The modern way to do something like that is a target property and a way to 
set the default.

See for example commit bd3496300262bd26073ce03e020731c592249148 (Refactor 
generation of shared library flags, 2012-05-30). The 
set(CMAKE_POSITION_INDEPENDENT_CODE ON) feature is enabled by 

 this->SetPropertyDefault("POSITION_INDEPENDENT_CODE", 0); 

in cmTarget.

For more/similar, see commit cd1fa537a03377974a4d0a27e592785f931a41e5 (Add a 
COMPILE_OPTION for a VISIBILITY_INLINES_HIDDEN target property., 2013-05-23) 
and 0e9f4bc00c6b26f254e74063e4026ac33b786513 (Introduce target property 
<LANG>_VISIBILITY_PRESET, 2013-05-18).

I added a patch to my clone with the start of 
CMAKE_CXX_COMPILE_OPTIONS_STD_CXX11 

 https://gitorious.org/~steveire/cmake/steveires-cmake/commits/std-cxx-target-property

but it didn't get anywhere yet: 

 http://thread.gmane.org/gmane.comp.programming.tools.cmake.devel/4106

4)

The COMPILE_OPTIONS for clang+apple might have to include -stdlib=libc++ for 
binary compatibility reasons if any of the dependencies use c++11 std 
library API in their interface and use libc++.

See what I wrote about that here:

 http://thread.gmane.org/gmane.comp.programming.tools.cmake.devel/5813

5)

This is not nice API:

 #if defined (CXXFEATURES_NULLPTR_FOUND)
     void *nix = nullptr;
 #else /* CXXFEATURES_NULLPTR_FOUND */
     void *nix = 0;
 #endif /* CXXFEATURES_NULLPTR_FOUND */


Much better would be:

     void *nix = CXXFEATURES_NULLPTR;

where -DCXXFEATURES_NULLPTR=0 or -DCXXFEATURES_NULLPTR=nullptr.

See what Qt does for other similar API decisions on what should be defined 
to something (like nullptr, constexpr, final etc), and what should be a 
'guard' define like above (eg lambdas, variadic templates etc).

Note also that by defining the CXXFEATURES_FINAL to something, you get to 
use the 'sealed' extension, which does the same thing, and works with 
VC2005. See qcompilerdetection.h.

6)

It is also unfortunate that because your solution is to define things on the 
command line, you can't easily define something for static_assert. See 
CMakeStaticAssert for example, which works for all compilers on the 
dashboard.

For that reason, I'll continue to recommend that anyone using Qt and CMake 
does not use your module, but instead uses the defines from Qt. Ditto for 
anyone using boost.

A missing piece that your module provides is determining the features at 
CMake time. However, if Qt is found, that can be done with a single 
try_compile of qglobal.h to get that information into a form usable to 
CMake. That's a feature I can add to Qt 5 soon, and something similar can be 
done to process the information from boost.config.

Thanks,

Steve