[cmake-developers] Please review CXXFeatures.cmake

[Stephen Kelly]

Rolf Eike Beer wrote:

>> 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.
> 
> The module returns just a list of CMake flags. How this is passed to the
> user (header, defines, whatever) is currently something the user must
> decide. I will not do anything fancy in the testcase for now.

Imagine I wanted to set Grantlee_FINAL to empty or final based on whether 
c++11 was active or not. How would I do that? 

I might do this:

 # This seems like an API smell. With g++ I want to wrap the add_definitions
 # in a condition for enabling c++11 at all. Does this mean that c++11 
 # features are not made available when using MSVC?
 if(CXX11_COMPILER_FLAGS)
   # Can't use add_compile_options as CXX11_COMPILER_FLAGS is a string, not 
   # a list.
   # This means that -std=c++11 is also passed when linking.
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CXX11_COMPILER_FLAGS}")

   if (CXXFeatures_class_override_final_FOUND)
     add_definitions(-DGrantlee_FINAL=final)
     set(_final_defined 1)
   endif() 
 endif()

 if (NOT _final_defined)
   add_definitions(-DGrantlee_FINAL=)
 endif()


Do you have any more-real-world examples of what code using your module 
would look like?

My c++ code would then look like:

 struct A Grantlee_FINAL
 {
   int data;
 };

However, now downstreams need to define Grantlee_FINAL to something in order 
to compile. We can help of course by putting Grantlee_FINAL in the 
INTERFACE_COMPILE_DEFINITIONS of Grantlee. 

However, what do we define it to? We need to define it based on the 
capabilities of the downstream. Ok, the way to do things like that in CMake 
is generator expressions.

 target_compile_definitions(Grantlee 
   INTERFACE Grantlee_FINAL=$<$<TARGET_PROPERTY:CXX11>:final>
 )

So, if the consumer has the CXX11 property ON, then it will be defined to 
'final'.

However, the problem is that there is no standard CXX11 target property. At 
best, I'd choose a property name which is a de-facto standard. See point 3:

 http://thread.gmane.org/gmane.comp.programming.tools.cmake.devel/6726/focus=7671


To repeat my point 5, the target_compile_definitions code might look more 
like this:

 set(msvc_genex $<STREQUAL:$<COMPILER_ID>,MSVC>)
 set(msvc_sealed_min $<NOT:$<VERSION_LESS:$<CXX_COMPILER_VERSION>,1400>>)
 set(msvc_sealed_max $<VERSION_LESS:$<CXX_COMPILER_VERSION>,1700>)
 set(msvc_sealed $<AND:${msvc_genex},${msvc_sealed_min},${msvc_sealed_max}>)
 set(_use_sealed $<AND:$<TARGET_PROPERTY:CXX11>,${msvc_sealed}>)
 set(_use_final $<AND:$<TARGET_PROPERTY:CXX11>,$<NOT:${msvc_sealed}>>)
 set(cxx11_final $<${_use_final}:final>$<${_use_sealed}:sealed>)

 target_compile_definitions(Grantlee 
   INTERFACE Grantlee_FINAL=${cxx11_final}
 )

See https://qt.gitorious.org/qt/qtbase/commit/37a660c594a


I really think that's something that should be solved by your module. I 
still think you should revisit my review mail on point 2 too.

Thanks,

Steve.