[PATCH] D12292: [CMake] merge add_compiler_rt_runtime and add_compiler_rt_darwin_runtime into a single function

[Chris Bieneman via llvm-commits]

beanz added a comment.

A few comments inline. I'll work up patches to address your feedback and send them out shortly.


================
Comment at: cmake/Modules/AddCompilerRT.cmake:63
@@ +62,3 @@
+function(add_compiler_rt_runtime name)
+  cmake_parse_arguments(LIB "SHARED;STATIC"
+    "OUTPUT_NAME;PARENT_TARGET"
----------------
samsonov wrote:
> I would leave SHARED/STATIC as `type` argument of `add_compiler_rt_runtime` function. Otherwise you can specify both STATIC and SHARED, which makes no sense. You should also diagnose if none is specified.
There are situations where we build both static and dynamic libs. tsan/dd/CMakeLists.txt is one example. I was hoping to collapse that all down to one call to `add_compiler_rt_runtime`

If you think it is better to support only one type I can do that. It will allow me to simplify some of the body of `add_compiler_t_runtime` too.

================
Comment at: cmake/Modules/AddCompilerRT.cmake:141
@@ -77,2 +140,3 @@
+      set_target_properties(${libname} PROPERTIES
         OUTPUT_NAME ${LIB_OUTPUT_NAME})
     endif()
----------------
samsonov wrote:
> This whole OUTPUT_NAME logic looks terrible now, especially assuming you can have several ${libname} libraries, but you pass a single OUTPUT_NAME for them. Ugh. Looks like it's only really used on Windows, maybe you will just adjust for naming difference on Windows and Linux at the place where you generate ${libname}, and get rid of this argument?
Yea... This is actually broken in my patches, and I think the behavior is broken. I'll try and revise it to be more sane.


http://reviews.llvm.org/D12292