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

[René J . V . Bertin]

rjvbb updated this revision to Diff 50812.
rjvbb added a comment.


  Now tested more exhaustively and with unittest.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16894?vs=50589&id=50812

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

AFFECTED FILES
  docs/module/ECMAddCompilerFlag.rst
  kde-modules/KDECompilerSettings.cmake
  kde-modules/KDEFrameworkCompilerSettings.cmake
  modules/ECMAddCompilerFlag.cmake
  tests/CMakeLists.txt
  tests/ECMAddCompilerFlag/CMakeLists.txt
  tests/ECMAddCompilerFlag/main.c

To: rjvbb, #build_system, kfunk
Cc: cgiboudeaux, dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, 
#build_system, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb set the repository for this revision to R240 Extra CMake Modules.

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

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

[René J . V . Bertin]

rjvbb added a comment.


  >   So according to you, this line is useful ? from my point of view, it's 
needless and just looks like a syntax error.
  
  Yes, it shows which compiler is being queried and for what. The line is going 
to be there anyway because CMake's Check*CompilerFlag macros don't have a QUIET 
option.
  
  We can discuss the exact dynamic variable name but since Cmake will not query 
the compiler twice for the same return variable the template should contain 
both the compiler ID and the flag being tested in order to cover all possible 
uses.
  I've considered trying to reset the cached result variable but we probably 
don't want to lose the benefits of that caching.

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

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

[Christophe Giboudeaux]

cgiboudeaux added a comment.


  In D16894#402901 , @rjvbb wrote:
  
  > > Forget that. The syntax is confusing, please remove this HASFLAG
  >
  > That I'm not going to do. The goal is to both to have useful feedback like 
below, and to avoid caching issues that would cause on the first query to be 
performed (if you were to use a single result variable):
  >
  >   -- Performing Test Clang++_ACCEPTS-Wvla
  >   ``
  
  
  So according to you, this line is useful ? from my point of view, it's 
needless and just looks like a syntax error.

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

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

[René J . V . Bertin]

rjvbb added a comment.


  > Forget that. The syntax is confusing, please remove this HASFLAG
  
  That I'm not going to do. The goal is to both to have useful feedback like 
below, and to avoid caching issues that would cause on the first query to be 
performed (if you were to use a single result variable):
  
-- Performing Test Clang++_ACCEPTS-Wvla
``

I decided against the idea of using a single macro name with a language 
parameter very early during development. It adds a parameter to each call or 
extra code to the macro to implement a default value, it can lead to errors 
that can remain undetected, and it deviates from the usual CMake syntax.

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

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

[Christophe Giboudeaux]

cgiboudeaux added inline comments.

INLINE COMMENTS

> ECMAddCompilerFlag.cmake:108
> +check_cxx_compiler_flag(${flag} ${HASFLAG})
> +if({${HASFLAG}})
> +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${flag}" 
> PARENT_SCOPE)

Forget that. The syntax is confusing, please remove this HASFLAG

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

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

[Christophe Giboudeaux]

cgiboudeaux added inline comments.

INLINE COMMENTS

> ECMAddCompilerFlag.cmake:108
> +check_cxx_compiler_flag(${flag} ${HASFLAG})
> +if({${HASFLAG}})
> +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${flag}" 
> PARENT_SCOPE)

if(HASFLAG)

you're testing the result, not the string

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

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

[Christophe Giboudeaux]

cgiboudeaux added a comment.


  In 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

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

[René J . V . Bertin]

rjvbb added a comment.


  >   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)?
  
  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.

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

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

[Christophe Giboudeaux]

cgiboudeaux added a comment.


  In D16894#402364 , @rjvbb wrote:
  
  >
  
  
  
  
  > Christophe: can you please be more specific why sphinx fails? Also, is 
there an existing unittest I can clone and adapt?
  
  There are tests for other ECM modules in the **tests** subdir.

INLINE COMMENTS

> rjvbb wrote in ECMAddCompilerFlag.cmake:1-25
> I'm guessing you meant "Please move [...]" and not that this would somehow 
> happen automagically(?)

Indeed.

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

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

[René J . V . Bertin]

rjvbb set the repository for this revision to R240 Extra CMake Modules.

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

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

[René J . V . Bertin]

rjvbb updated this revision to Diff 50589.
rjvbb marked an inline comment as done.
rjvbb added a comment.


  Updated as requested.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16894?vs=50451&id=50589

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

AFFECTED FILES
  docs/module/ECMAddCompilerFlag.rst
  kde-modules/KDECompilerSettings.cmake
  kde-modules/KDEFrameworkCompilerSettings.cmake
  modules/ECMAddCompilerFlag.cmake

To: rjvbb, #build_system, kfunk
Cc: cgiboudeaux, dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, 
#build_system, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb marked 9 inline comments as done.
rjvbb added inline comments.

INLINE COMMENTS

> cgiboudeaux wrote in ECMAddCompilerFlag.cmake:1-25
> Shall be moved after the module documentation

I'm guessing you meant "Please move [...]" and not that this would somehow 
happen automagically(?)

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

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

[René J . V . Bertin]

rjvbb added a comment.


  >   Right, but I was saying all this because I think IF_SUPPORTED (the 
keyword in the arguments) should be SUPPORTED_IF.
  
  Doh?! I thought that was the case, maybe I just restored the wrong way after 
the CONDITION experiment.
  
  Christophe: can you please be more specific why sphinx fails? Also, is there 
an existing unittest I can clone and adapt?

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

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

[David Faure]

dfaure added a comment.


  In D16894#402209 , @rjvbb wrote:
  
  > >   In that sentence, one can read "if supported" for the macro name, ...
  >
  > That was my idea too, and the reason the macro ends in "_if_supported".
  
  
  Right, but I was saying all this because I think IF_SUPPORTED (the keyword in 
the arguments) should be 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

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

[Christophe Giboudeaux]

cgiboudeaux added inline comments.

INLINE COMMENTS

> ECMAddCompilerFlag.cmake:1-25
> +#=
> +# Copyright 2018  René J.V. Bertin 
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions
> +# are met:
> +#

Shall be moved after the module documentation

> ECMAddCompilerFlag.cmake:27-29
> +include(CMakeParseArguments)
> +include(CheckCXXCompilerFlag)
> +include(CheckCCompilerFlag)

Shall be moved after the license block

> ECMAddCompilerFlag.cmake:30-58
> +
> +# ECM_ADD_CXX_COMPILER_FLAGS_IF_SUPPORTED(FLAGS 
> +# [IF_SUPPORTED ]
> +# [QUERY_IF ]
> +# )
> +# add  or  to CMAKE_CXX_FLAGS if the compiler supports them.
> +# Support is determined by the IF_SUPPORTED expression if provided or by

Sphinx cannot parse this doc.

Also don't forget to create ECMAddCompilerFlag.rst in docs/modules/

> ECMAddCompilerFlag.cmake:58
> +# [QUERY_IF ]
> +# )
> +

Missing "Since 5.xx"

> ECMAddCompilerFlag.cmake:61
> +
> +function (ECM_ADD_CXX_COMPILER_FLAGS_IF_SUPPORTED)
> +set(_OPTIONS_ARGS)

Unneeded extra space

> ECMAddCompilerFlag.cmake:66
> +
> +cmake_parse_arguments(EASCXXFLAGS "${_OPTIONS_ARGS}" 
> "${_ONE_VALUE_ARGS}" "${_MULTI_VALUE_ARGS}" ${ARGN} )
> +if(NOT EASCXXFLAGS_FLAGS)

Unneeded extra space

> ECMAddCompilerFlag.cmake:68
> +if(NOT EASCXXFLAGS_FLAGS)
> +message( FATAL_ERROR "ecm_add_cxx_compiler_flags_if_supported: 
> 'FLAGS' is a required argument." )
> +endif()

Unneeded extra spaces

> ECMAddCompilerFlag.cmake:96
> +
> +function (ECM_ADD_C_COMPILER_FLAGS_IF_SUPPORTED)
> +set(_OPTIONS_ARGS)

Unneeded extra space

> ECMAddCompilerFlag.cmake:101
> +
> +cmake_parse_arguments(EASCFLAGS "${_OPTIONS_ARGS}" "${_ONE_VALUE_ARGS}" 
> "${_MULTI_VALUE_ARGS}" ${ARGN} )
> +if(NOT EASCFLAGS_FLAGS)

Unneeded extra space

> ECMAddCompilerFlag.cmake:103
> +if(NOT EASCFLAGS_FLAGS)
> +message( FATAL_ERROR "ecm_add_c_compiler_flags_if_supported: 'FLAGS' 
> is a required argument." )
> +endif()

Unneeded extra space

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

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

[Christophe Giboudeaux]

cgiboudeaux added a comment.


  PS: don't forget the unit test for the new module.

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

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

[René J . V . Bertin]

rjvbb added a comment.


  >   In that sentence, one can read "if supported" for the macro name, ...
  
  That was my idea too, and the reason the macro ends in "_if_supported".

REPOSITORY
  R240 Extra CMake Modules

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

To: rjvbb, #build_system, kfunk
Cc: dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, 
michaelh, ngraham, bruns

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

[David Faure]

dfaure added a comment.


  > SUPPORTED_IF : add the flag(s) if the expression is true?
  
  Yes.
  
  > TRY_IF: query the compiler if the expression is true?
  
  Yes.
  
  > How would that intersect with the SUPPORTED_IF test?
  
  In addition.
  
  AFAICS this is what you implemented, so I like it. I just find SUPPORTED_IF 
(as an intro to the condition) more readable than IF_SUPPORTED.
  
  The macro name seems fine to me.
  In plain English one would say "Add this flag if it's supported. It's 
supported for sure if condition A is true. Also try it if condition B is true, 
since it might work then, but we can't be sure".
  In that sentence, one can read "if supported" for the macro name, "SUPPORTED 
IF" for the first condition, and "TRY IF" for the second condition.

REPOSITORY
  R240 Extra CMake Modules

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

To: rjvbb, #build_system, kfunk
Cc: dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, 
michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb updated this revision to Diff 50451.
rjvbb added a comment.


  This follows David's suggestion, but using `QUERY_IF` instead of the 
suggested `TRY_IF` to make it clear that this parameter controls the querying 
of the compiler.
  I haven't yet tested the new logic exhaustively but the as far as I can tell 
the macro behaves as intended as used in the two compiler settings modules.
  
  I've simplified the `QUERY_IF` conditions in those modules to just "if 
APPLE". Querying systematically on APPLE seems reasonable because the 
likelihood that a non-clang compiler is used for building is very small and 
when  it happens we'd probably be dealing with a non-GNU compiler.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16894?vs=50399&id=50451

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

AFFECTED FILES
  kde-modules/KDECompilerSettings.cmake
  kde-modules/KDEFrameworkCompilerSettings.cmake
  modules/ECMAddCompilerFlag.cmake

To: rjvbb, #build_system, kfunk
Cc: dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, 
michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb set the repository for this revision to R240 Extra CMake Modules.

REPOSITORY
  R240 Extra CMake Modules

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

To: rjvbb, #build_system, kfunk
Cc: dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, 
michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added a comment.


  Usually if you have a conditional behaviour the associated condition 
specifies when to trigger it, no?
  You're right that the names don't suggest exactly how the condition is being 
evaluated (with extra checks or not), but that was also a bit the idea.
  Don't bother the user with such details, just provide a macro that will add 
the flag(s) if they are supported, with an optional conditional expression that 
can make things faster.
  
  Not that this optimisation makes a huge difference in my experience, so the 
already suggested simplification of just querying the compiler for everything 
isn't completely off the table either AFAIAC.
  
  >   In summary, I suggest having both SUPPORTED_IF and TRY_IF, and moving 
AppleClang magic out of the function.
  
  So:
  
  SUPPORTED_IF : add the flag(s) if the expression is true?
  TRY_IF: query the compiler if the expression is true? How would that 
intersect with the SUPPORTED_IF test?
  
  What about the macro name itself in that case?
  
  What bothers me here is that it puts the responsability to invoke magic or 
not back in the hands of the users, most of whom cannot test (directly) on the 
platform where this is needed. That could well open the door to new issues, 
either when a new problematic flag is added, or if someone thinks that some 
cleanup is in order and safe.
  
  Counter proposal: add a SUPPORTED_BY or SUPPORTING_COMPILERS parameter that 
take a list of compiler IDs for flags like -pedantic that are supported by all 
versions and can thus be added with only a simple compiler check. The question 
then is if an error should be raised if both parameters are specified?

REPOSITORY
  R240 Extra CMake Modules

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

To: rjvbb, #build_system, kfunk
Cc: dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, 
michaelh, ngraham, bruns

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

[David Faure]

dfaure added a comment.


  Ah. You meant an "OR", I thought it was an "AND".   (as in `our known 
selection of compilers OR/AND appleclang supports it`)
  But things are certainly not clearer now with the name CONDITION, which 
doesn't imply either one.
  
  Aside from the AppleClang issue, I've had cases where a compiler flag was 
added in an unreleased version of the compiler, so it makes sense to me to have 
a way to have a TRY_IF condition.
  At the same time, I guess we can have SUPPORTED_IF for the cases where we 
know it's supported and we want to save time by not even checking it.
  Then one could say "TRY_IF apple and clang" for the flags that we want to 
double-check on appleclang, instead of the function having this black magic 
hardcoded inside.
  And this way we don't need to check flags that work on all versions of clang 
like -pedantic.
  
  In summary, I suggest having both SUPPORTED_IF and TRY_IF, and moving 
AppleClang magic out of the function.

REPOSITORY
  R240 Extra CMake Modules

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

To: rjvbb, #build_system, kfunk
Cc: dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, 
michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb set the repository for this revision to R240 Extra CMake Modules.

REPOSITORY
  R240 Extra CMake Modules

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

To: rjvbb, #build_system, kfunk
Cc: dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, 
michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb updated this revision to Diff 50399.
rjvbb added a comment.


  Renamed macro and parameter names as announced in my last comment.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16894?vs=45786&id=50399

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

AFFECTED FILES
  kde-modules/KDECompilerSettings.cmake
  kde-modules/KDEFrameworkCompilerSettings.cmake
  modules/ECMAddCompilerFlag.cmake

To: rjvbb, #build_system, kfunk
Cc: dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, 
michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added a comment.


  In D16894#400949 , @dfaure wrote:
  
  > This makes sense to me. Just the name "SUPPORTED_IF" is strange, when 
reading that, one thinks "well, if we know the compiler flag is supported, why 
are we testing that it is?".
  
  
  I agree it looks strange, but apparently the combination with the macro name 
isn't as unambiguous as I hoped.
  
  The idea is: add the given option(s) but only If they are Supported by the 
compiler ... and they are Supported If the given expression is true.
  
  > I think this should be something like TRY_IF.
  
  That reads nicer, but isn't exact either because a priori there will be no 
more trying if the expression is true...
  
  I'll change the macro name to `ECM_ADD__FLAGS_CONDITIONALLY` and the 
parameter name to `CONDITION`, let's see how that works out.
  
  > Then it's clearer that no harm will occur if we set a too low compiler 
version after TRY_IF, it's just an optimization to avoid e.g. testing all gcc 
flags on MSVC and vice-versa.
  
  Uhm? That was not the intention. Currently if a TRY_IF condition is present 
then it is the only criterium used, UNLESS the compiler is Clang/AppleClang on 
APPLE. In that latter case the compiler is queried; this is also the case when 
no conditional expression is specified.
  
  I can of course inverse that logic, and make the condition a filter for when 
to query the compiler. At first sight I'd say that the conditional expressions 
won't need adapting but I have a feeling it might not be as simple.

REPOSITORY
  R240 Extra CMake Modules

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

To: rjvbb, #build_system, kfunk
Cc: dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, 
michaelh, ngraham, bruns

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

[David Faure]

dfaure added a comment.


  This makes sense to me. Just the name "SUPPORTED_IF" is strange, when reading 
that, one thinks "well, if we know the compiler flag is supported, why are we 
testing that it is?". I think this should be something like TRY_IF.
  Then it's clearer that no harm will occur if we set a too low compiler 
version after TRY_IF, it's just an optimization to avoid e.g. testing all gcc 
flags on MSVC and vice-versa.

REPOSITORY
  R240 Extra CMake Modules

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

To: rjvbb, #build_system, kfunk
Cc: dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, 
michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb set the repository for this revision to R240 Extra CMake Modules.

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

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

[René J . V . Bertin]

rjvbb updated this revision to Diff 45786.
rjvbb added a comment.


  This implements and uses my idea of an 
`ecm_add__compiler_flags_if_supported`  function set for C and C++. It 
uses compiler ID+version conditions to determine if flag(s) are supported when 
those conditions are known and reliable - otherwise and only then does it 
resort to querying the compiler.
  
  I do not particularly care whether or not the KDE*CompilerSettings modules 
use the `SUPPORTED_IF` feature but I think it should be there as a courtesy to 
devs who have reasons to avoid querying the compiler (and for compilers where 
CMake's querying algorithm doesn't work).

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16894?vs=45567&id=45786

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

AFFECTED FILES
  kde-modules/KDECompilerSettings.cmake
  kde-modules/KDEFrameworkCompilerSettings.cmake
  modules/ECMAddCompilerFlag.cmake

To: rjvbb, #build_system, kfunk
Cc: kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, 
michaelh, ngraham, bruns

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( [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 
# [SUPPORTED_IF ]
# )
# add  or  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

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

[René J . V . Bertin]

rjvbb added a comment.


  >   Thus these places need to be turned into:
  >   
  > ...
  > if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang" AND 
CMAKE_CXX_COMPILER_VERSION VERSION_LESS "3.8")
  > elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" AND 
CMAKE_CXX_COMPILER_VERSION VERSION_LESS "8.1.0")
  
  As you know this doesn't work reliably: whether it works or not depends on 
cmake version and on CMP0025 - and thus requires all projects to set that 
policy before calling `project`. I don't think that's an option, nor is 
expecting Mac users to patch all toplevel CMake files of projects they want to 
work with (or patch CMake so it sets the policy which is basically what I do on 
my end). I also vaguely recall that cmake used the dotless AppleClang version 
in earlier versions (= 810 instead of 8.1.0).
  
  Besides, all those repeated separate ifs and elses do not make the code 
easier to read or parse. Isn't it the whole purpose of having macros to reduce 
code duplication and to hide complexity in a single location so that the 
intended behaviour becomes easier to follow?
  
  This makes Aleix's suggestion just to use `compiler_check_flag` much 
more appealing, despite the cost and the fact it's so easy to break.
  
  > Statements like `CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND NOT 
CMAKE_CXX_COMPILER_VERSION VERSION_LESS 3.5` do not make sense.
  
  That's a bit too strong. They make sense everywhere except on APPLE (and even 
there they can be reliable).
  
  > If you're unsure in which version of AppleClang a certain feature was 
introduced then:
  
  This is the gist of what I'm doing, except that I do not want to rely on 
Apple clang being reported as AppleClang (= CMP0025 set to NEW).
  
  > This "let's abuse ${ARGN} as code to be evaluated later" is pretty ugly.
  
  It comes from a cmake ML post by a cmake dev (Brad Kind IIRC) so I guess it's 
sanctioned ugliness (as so much in cmake syntax). The boolean operators are 
only available in the IF functions so there aren't much options if you don't 
want to use CmakeParseArguments. I'm already looking at doing just that for a 
version that will look like `kde_add_supported_compiler_flags(FLAGS  
SUPPORTED_IF )`. The names (macro and keywords) are open for 
discussion of course, and so is the addition of additional options.
  
  I'll look at kdevelop's versions too. I have no real objection to favour my 
own code over them if they work reliably. But now that you mention kdevelop: 
I've long been forced to use stock compilers to build KDevelop (on Mac). I 
always put that off to my AppleClang being too old but with this new knowledge 
I suspect it's simply because something goes wrong in the compiler detection.
  
  > 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.
  
  Erm, no?! The macro name suggests it runs a check, which is true. It doesn't 
say which check...
  (FWIW, `cmake__compiler_check_flag` is equally misleading: it doesn't 
check `${flag}` but `"${CMAKE_CXX_FLAGS} ${flag}"`)

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

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( [CXX_ONLY])
#   add_target_compile_flag_if_supported( 
 )
  
  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

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

[René J . V . Bertin]

rjvbb edited the test plan for this revision.

REPOSITORY
  R240 Extra CMake Modules

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

To: rjvbb, #build_system
Cc: apol, kde-frameworks-devel, kde-buildsystem, #build_system, michaelh, 
ngraham, bruns

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

[René J . V . Bertin]

rjvbb retitled this revision from "[ECM] use a macro to test compiler flag 
support" to "[ECM] use a macro to add compiler flags conditionally".
rjvbb set the repository for this revision to R240 Extra CMake Modules.

REPOSITORY
  R240 Extra CMake Modules

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

To: rjvbb, #build_system
Cc: apol, kde-frameworks-devel, kde-buildsystem, #build_system, michaelh, 
ngraham, bruns