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

Revision history for this message
Sam Spilsbury (smspillaz) 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.

Yes, that is ugly.

The reason I did it that way was because CMake seems to have a preference for storing the include dirs and link libraries on a per-directory basis rather than a per-target basis. Or at least, I was hitting problems where I would end up with absurdly long linker lines because it was pulling in all the stuff from the directories before it.

I'll need to have another look at whether or not I was just hacking around some problem that I may have fixed later on.

>
> On this line the word "library" is misspelt:
>
> list (APPEND ${_full_prefix}_LINK_FLAGS ${${_full_prefix}_LIBARY_FLAGS})

ack.

« Back to merge proposal