Replace CMAKE_CXX_FLAGS with better option (warning config)

[ricab]

(issue corresponding to review suggestion here)

CMAKE_CXX_FLAGS is used to configure compiler warnings in cmake as part of the testing procedure, (following the this advice). I am not aware of a warning API in cmake and I know of no option that does not involve passing explicit flags to the compiler. So, AFAIU, better options to configure compiler warnings would still require explicit flags.

With that in mind, I see 2 contending alternatives to the current approach:

• use target_compile_options on each target
• use add_compile_options once

To be discussed. Suggestions of better options are welcome.

[ricab]

@Quincunx271, could you please provide or point me to something explaining why this is a bad habit even when compiler flags are needed? Will clients somehow be affected when #3 is done? And what better option did you envisage?

[Quincunx271]

First off, I am not really that experienced with CMake, but I do have an idea of what the best practices are. It's totally possible that there are better practices / reasons than what I know.

If you want the library to be consumable via add_subdirectory(...), you do not want to pollute things like CMAKE_CXX_FLAGS. Also, I personally find it much easier to understand when CMAKE_CXX_FLAGS isn't explicitly touched in the CMakeLists.txt

You could use add_compile_options, but I would personally use target_compile_options. You could write a cmake function to do it for you if you don't want to repeat yourself:

function(scope_guard_compile_options TARGET)
  target_compile_options(${TARGET}
    # options go here
  )
endfunction()

For setting up warnings, I usually do this:

target_compile_options(my-target
  PRIVATE
    $<$<CXX_COMPILER_ID:Gnu>:
      # g++ warning flags
    >
    $<$<CXX_COMPILER_ID:Clang>:
      # clang warning flags
    >
    $<$<CXX_COMPILER_ID:MSVC>:
      # MSVC warning flags
    >
)

[OvermindDL1]

For 'best practices' from the big cmake dev's themselves see: https://cgold.readthedocs.io/en/latest/

[ricab]

@OvermindDL1: thanks again for the pointer. I did not find anything relevant to this issue there though.

@Quincunx271:

First off, I am not really that experienced with CMake, but I do have an idea of what the best practices are. It's totally possible that there are better practices / reasons than what I know.

Pretty much the same here. I share the impression that CMake abstractions are better than direct compiler flags in general, but I don't see an alternative here. And I looked at the time... Perhaps add_compile_options or target_compile_options are more elegant than changing CMAKE_CXX_FLAGS... I wish I could see the right argument for that.

If you want the library to be consumable via add_subdirectory(...), you do not want to pollute things like CMAKE_CXX_FLAGS.

Hmm, well it was not really meant for that – it is a top-level CMakeLists.txt which would require other changes anyway to be used like that. BTW, I intend to configure warnings only in the BUILD_TESTING case when #3 is done.

easier to understand when CMAKE_CXX_FLAGS isn't explicitly touched in the CMakeLists.txt

Again, I agree in general, but because there are usually better cmake abstractions. Perhaps add_compile_options is more elegant here, but it feels pretty much like a matter of taste.

I would personally use target_compile_options

Come to think of it, add_compile_options seems preferable to configuring each individual target the same way, as it conveys right away that the options are to be applied in all targets, wouldn't you say? What would be the point of having a function that was called for all targets anyway?

[OvermindDL1]

Honestly, as long as the library is accessible via something like Hunter (or any of the other cmake dependency systems, though hunter is the only one that only requires cmake and nothing else) then it becomes trivial to use. For that I think all it needs is to be able to 'install' properly with all necessary header files and config files and such available.

[ricab]

@OvermindDL1 point taken