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<lang>_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 <flags> SUPPORTED_IF <conditions>)`. 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_<lang>_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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20181116/593dcef2/attachment.html>