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

[Christophe Giboudeaux]

cgiboudeaux added a comment.


  In D16894#402701 <https://phabricator.kde.org/D16894#402701>, @rjvbb wrote:
  
  > >   There are tests for other ECM modules in the **tests** subdir.
  >
  > That's not the expected answer, so let me rephrase: which existing test can I clone and adapt (which is about the only thing I know how to do in this domain)?
  
  
  And the answer is the same, there are examples in the tests/ folder.
  
  > I'm going to need a hand here if not only because the only kind of testing that makes sense to me is checking manually with a selection of the compilers you happen to have installed. Anything automatic I can think of would not be able to do much more than taking the resulting CMAKE_??_FLAGS variable and check if the compiler indeed accepts all the arguments. That's basically just repeating the same tests already performed in the macro you're supposed to be testing and thus as much (if not more) a test of the input logic (the conditional expression(s)) as of the macro.
  
  The macro has different parameters.
  
  Things you can test:
  
  - Compiler flags accepted by all compilers (-Wall, -Wextra...)
  - flags only accepted by a given compiler. You can then check that it's not added by mistake to unsupported platforms
  
  About the code itself:
  The two functions are clones, this can be avoided by only having one ECM_ADD_COMPILER_FLAG function and a LANGUAGE argument.
  This has 2 benefits:
  
  - The module name matches the function name
  - it's shorter than ecm_add_cxx_compiler_flag_if_supported and easier to remember.

INLINE COMMENTS

> ECMAddCompilerFlag.cmake:94
> +    # if the user provided conditions, evaluate them now to simplify things later
> +    if(EASCXXFLAGS_IF_SUPPORTED AND (${EASCXXFLAGS_IF_SUPPORTED}))
> +        set(EASCXXFLAGS_is_supported ON)

EASCXXFLAGS_SUPPORTED_IF

> ECMAddCompilerFlag.cmake:98
> +    if((EASCXXFLAGS_QUERY_IF AND (${EASCXXFLAGS_QUERY_IF}))
> +        OR (NOT EASCXXFLAGS_IF_SUPPORTED AND NOT EASCXXFLAGS_QUERY_IF))
> +        set(EASCXXFLAGS_needs_query ON)

EASCXXFLAGS_SUPPORTED_IF

> ECMAddCompilerFlag.cmake:129
> +    # if the user provided conditions, evaluate them now to simplify things later
> +    if(EASCFLAGS_IF_SUPPORTED AND (${EASCFLAGS_IF_SUPPORTED}))
> +        set(EASCFLAGS_is_supported ON)

EASCFLAGS_SUPPORTED_IF

> ECMAddCompilerFlag.cmake:133
> +    if((EASCFLAGS_QUERY_IF AND (${EASCFLAGS_QUERY_IF}))
> +        OR (NOT EASCFLAGS_IF_SUPPORTED AND NOT EASCFLAGS_QUERY_IF))
> +        set(EASCFLAGS_needs_query ON)

EASCFLAGS_SUPPORTED_IF

REPOSITORY
  R240 Extra CMake Modules

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

To: rjvbb, #build_system, kfunk
Cc: cgiboudeaux, dfaure, 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-frameworks-devel/attachments/20190131/94f52436/attachment-0001.html>