D16894: [ECM] use a macro to add compiler flags conditionally

[Kevin Funk]

kfunk requested changes to this revision.
kfunk added a comment.
This revision now requires changes to proceed.


  I don't like the hiding of the if-branches as argument to macro. We shouldn't to this as it makes the code harder to understand.
  
  The general issue with the existing code here is: Statements like `CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND NOT CMAKE_CXX_COMPILER_VERSION VERSION_LESS 3.5` do not make sense. If we check the compiler version, then we need to differentiate between Clang and AppleClang (cf. something like https://github.com/Microsoft/LightGBM/blob/master/CMakeLists.txt#L20).
  
  Thus these places need to be turned into:
  
    ...
    if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS "3.8")
      ...
    endif()
    elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS "8.1.0")
      ...
    endif()
  
  If you're unsure in which version of AppleClang a certain feature was introduced then:
  
    ...
    elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS "8.1.0")
      add_compile_flag_if_supported(...)
    endif()
  
  Please check out the functions provided in: kdevelop.git:cmake/modules/KDevelopMacrosInternal.cmake
  
    #   add_compile_flag_if_supported(<flag> [CXX_ONLY])
    #   add_target_compile_flag_if_supported(<target> <INTERFACE|PUBLIC|PRIVATE> <flag>)
  
  They are more clean than your implementation, I think it would make sense to actually add them to KDECompilerSettings.cmake and prefix them with `kde_`.

INLINE COMMENTS

> KDECompilerSettings.cmake:202
> +    # is expected to support _FLAG.
> +    if (${ARGN})
> +        if(APPLE AND CMAKE_CXX_COMPILER_ID MATCHES "Clang")

This "let's abuse ${ARGN} as code to be evaluated later" is pretty ugly. The previous version (with the if-statements at  the caller side) is way more clean and understandable.

> KDECompilerSettings.cmake:203
> +    if (${ARGN})
> +        if(APPLE AND CMAKE_CXX_COMPILER_ID MATCHES "Clang")
> +            # Clang on APPLE needs verification because of Apple's

I don't really understand why this branch is needed. The macro name name suggests it does the compile check on all compilers. Again very misleading.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D16894

To: rjvbb, #build_system, kfunk
Cc: kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-buildsystem/attachments/20181116/e52bab2c/attachment-0001.html>