[LLVMdev] [llvm] r201072 - [CMake] Introduce llvm_add_library().

[NAKAMURA Takumi]

Steve, excuse me to respond you partially.

2014-02-10 18:54 GMT+09:00 Stephen Kelly <steveire at gmail.com>:
> NAKAMURA Takumi wrote:
>
>> [CMake] Introduce llvm_add_library().
>
> I recommend moving away from wrappers like this. They indicate that either
> CMake is not providing the interfaces needed, or not propagating them, or
> that they exist but are not used.
>
> Such wrappers don't parse the arguments in the same way as the wrapped
> command etc. Wrappers are not good API proxies. Additionally, you put people
> who know cmake already at a disadvantage because you make the code look like
> 'not cmake code' by using llvm_add_library instead of add_library, the cmake
> command. Making that wrapper macro also invoke target_link_libraries looks
> odd to someone familiar with cmake.
>
> I don't fully know what this wrapper is needed for, but I looked at it a
> bit.
>
>   if(ARG_MODULE)
>     set_property(TARGET ${name} PROPERTY SUFFIX ${LLVM_PLUGIN_EXT})
>   endif()
>
>   if(ARG_SHARED)
>     if (MSVC)
>       set_target_properties(${name}
>         PROPERTIES
>         IMPORT_SUFFIX ".imp")
>     endif ()
>   endif()
>
>
> CMake provides CMAKE_SHARED_MODULE_SUFFIX and CMAKE_IMPORT_LIBRARY_SUFFIX.
> Can you set those at directory scope instead of setting the target property
> in the macro?

Reasonable. They could be put into common configurator, HandleLLVMOptions.
For now in r201072, I simply gather common logics around there.
I'd be happy if llvm_add_library became simpler, and I will work on.

> Anyway, just a note to consider moving instead in a direction of fewer
> wrappers, rather than more. I'm not familiar enough with the llvm
> buildsystem to know why you're doing some of the things you're doing, but if
> you tell us about the gaps in the cmake offering we can try to fill them.
>
> Thanks,
>
> Steve.