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, bruns
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 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=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
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
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
