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

[Stephen Kelly]

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?


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.