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.
--
Powered by www.kitware.com
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
