Code review comment for lp:~compiz-team/compiz-core/compiz-core.fix_883102

Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

The CMake functions seem to be duplicating a lot of stuff that CMake already does. For example there is a lot of code to build compiler flag prefixes which you really should not need to do. As an example is this line:

list (APPEND ${_prefix}_LIBRARY_LDFLAGS -l${_library})

It seems (not having read all of the build system code thoroughly) that these are then added to a target with set_target_properties. You really should not need to do this. Simply do target_link_libraries(foo library_of_bar). It's the same, but portable and consistency checked by CMake.

In the same vein, why are linker flags set with set_target_properties and not target_link_libraries? The arguments to the latter can be linker flags and they are handled appropriately by CMake. The same applies for include paths and other properties. Why have a function for include expansion (that basically just adds a "-I" to a list of strings) when all you would need to do is include_directories(${foobar})? These definitions do not propagate to parent directories so you can just set them in a test subdirectory and they remain isolated.

On this line the word "library" is misspelt:

list (APPEND ${_full_prefix}_LINK_FLAGS ${${_full_prefix}_LIBARY_FLAGS})

« Back to merge proposal