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

[René J.V. Bertin]

rjvbb added a comment.


  Something side-ways related: I went down this hole because cmake's `generate_export_header` failed because of an unsupported flag that was added.
  Regardless of how we implement things here, shouldn't there be something like `ecm_generate_export_header` which empties CMAKE_CXX_FLAGS temporarily because calling CMake's version and then restores the variable? There's no feedback at all in this function, the generated export header just contains dummy EXPORT macros, leaving the user to wonder why the linker fails. Or should the visibility flags also be set conditionally, after setting all other compiler options?
  
  > 1. add_compile_flag_if_supported(<flag> [CXX_ONLY])
  
  So that is basically just a wrapper around `compile_check_cxx_compiler_flag` with a bit of icing (consistent cache variables, which my approach also has, and a (cripped!) C++-only option).
  Issues I see:
  
  1- these will fail if someone adds an unsupported flag for which the compiler emits a warning, potentially breaking a build that would otherwise succeed. You could argue "just don't add unsupported options then". That may still be acceptable for individual projects, but I think it's not a good idea nonetheless. Build systems (and thus the ECM) should be as fool-proof as possible, and using ID+version checks can help here.
  2- (citing): "The macro name name suggests it does the compile check on all compilers."
  3- this assumes that the C++ compiler accepts all options accepted by all compilers (without emitting warnings), which is not true in practice
  
  Here's a counter proposal that should be clear in its intention and implementation (clear though a bit long and complex because covering all {b,c}ases):
  
    include(CMakeParseArguments)
    
    # ECM_ADD_CXX_COMPILER_FLAGS_IF_SUPPORTED(FLAGS <flag|flags>
    #     [SUPPORTED_IF <conditional expression>]
    # )
    # add <flag> or <flags> to CMAKE_CXX_FLAGS is the compiler supports them.
    # Support is determined by the SUPPORTED_IF expression if provided or by
    # querying the compiler directly otherwise. The compiler is also queried
    # when using a Clang C++ compiler on APPLE.
    # examples:
    # Check 
    # ecm_add_cxx_compiler_flags_if_supported(FLAGS -a -b -c SUPPORTED_IF CMAKE_C_COMPILER_ID STREQUAL "GNU" OR CMAKE_C_COMPILER_ID MATCHES "Clang")
    # ecm_add_cxx_compiler_flags_if_supported(FLAGS -d -e -f)
    
    function (ECM_ADD_CXX_COMPILER_FLAGS_IF_SUPPORTED)
        set(_OPTIONS_ARGS)
        set(_ONE_VALUE_ARGS)
        set(_MULTI_VALUE_ARGS FLAGS SUPPORTED_IF)
    
        cmake_parse_arguments(EASCXXFLAGS "${_OPTIONS_ARGS}" "${_ONE_VALUE_ARGS}" "${_MULTI_VALUE_ARGS}" ${ARGN} )
        if(NOT EASCXXFLAGS_FLAGS)
            message( FATAL_ERROR "ecm_add_cxx_compiler_flags_if_supported: 'FLAGS' is a required argument." )
        endif()
        # if the user provided supported_if conditions, evaluate them now:
        if (NOT EASCXXFLAGS_SUPPORTED_IF OR (${EASCXXFLAGS_SUPPORTED_IF}))
            # without conditions, or with Clang on APPLE we'll need to ask the compiler directly.
            # (Clang on APPLE needs verification because of Apple's llvm versions which cannot be
            # (matched easily to stock llvm versions.
            if(NOT EASCXXFLAGS_SUPPORTED_IF OR (APPLE AND CMAKE_CXX_COMPILER_ID MATCHES "Clang"))
                # one flag at a time:
                foreach(flag IN ITEMS ${EASCXXFLAGS_FLAGS})
                    # use a standardised and informative cached test variable
                    set(HASFLAG "${CMAKE_CXX_COMPILER_ID}++_ACCEPTS${flag}")
                    check_cxx_compiler_flag(${flag} ${HASFLAG})
                    if ({${HASFLAG}})
                        set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${flag}" PARENT_SCOPE)
                    endif()
                endforeach()
            else()
                # all flags can be appended at once
                string(REPLACE ";" " " FLAGS "${EASCXXFLAGS_FLAGS}")
                set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${FLAGS}" PARENT_SCOPE)
            endif()
        endif()
    endfunction()
  
  Using a function here to keep temp. variables local. And using the ECM "namespace" because there's nothing KDE specific in this.

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/5c81920c/attachment-0001.html>