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

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=50812 REVISION DETAIL https://phabricator.kde.org/D16894 AFFECTED FILES

D16894: [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, kfunk Cc: cgiboudeaux, dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system,

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

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

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

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

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

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

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

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,

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

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

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

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

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

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

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

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

D16894: [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, kfunk Cc: cgiboudeaux, dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system,

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

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=50589 REVISION DETAIL https://phabricator.kde.org/D16894 AFFECTED FILES

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

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

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

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

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

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

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

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 > +#

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

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,

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

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,

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

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

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

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

D16894: [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, kfunk Cc: dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, michaelh, ngraham,

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

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

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

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

D16894: [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, kfunk Cc: dfaure, kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, michaelh, ngraham,

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

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=50399 REVISION DETAIL https://phabricator.kde.org/D16894 AFFECTED FILES

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

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

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

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

D16894: [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, kfunk Cc: kfunk, apol, kde-frameworks-devel, kde-buildsystem, #build_system, michaelh, ngraham, bruns

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

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

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

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

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

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

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

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

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

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

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