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

[Alexey Samsonov via llvm-commits]

samsonov accepted this revision.
samsonov added a comment.
This revision is now accepted and ready to land.

LGTM


================
Comment at: lib/asan/CMakeLists.txt:192
@@ -188,4 +191,3 @@
                 ${VERSION_SCRIPT_FLAG}
-      DEFS ${ASAN_DYNAMIC_DEFINITIONS})
-    target_link_libraries(clang_rt.asan-dynamic-${arch} ${ASAN_DYNAMIC_LIBS})
-    add_dependencies(asan clang_rt.asan-dynamic-${arch})
+      LINKLIBS ${ASAN_DYNAMIC_LIBS}
+      DEFS ${ASAN_DYNAMIC_DEFINITIONS}
----------------
LINK_LIBS

================
Comment at: lib/profile/CMakeLists.txt:21-26
@@ -21,8 +20,8 @@
 else()
-  foreach(arch ${PROFILE_SUPPORTED_ARCH})
-    add_compiler_rt_runtime(clang_rt.profile-${arch} ${arch} STATIC
-      CFLAGS -fPIC
-      SOURCES ${PROFILE_SOURCES})
-    add_dependencies(profile clang_rt.profile-${arch})
-  endforeach()
+  add_compiler_rt_runtime(clang_rt.profile
+    STATIC
+    ARCHS ${PROFILE_SUPPORTED_ARCH}
+    CFLAGS -fPIC
+    SOURCES ${PROFILE_SOURCES}
+    PARENT_TARGET profile)
 endif()
----------------
Thank you for bringing this up, as it seems it to be the main source of confusion in the review thread (and sorry for not making my point clear). As I understand it now, you want to:
1) implement and commit add_compiler_rt_runtime with all the features you find useful (LINK_LIBS, PARENT_TARGET, multi-arch support)
2) exercise each of new features in 1-2 places to have them tested, but keep the change small.
3) (in a follow-up change) migrate everyone else to use new features.

While I was more used to (and tried to push you in that direction) a different approach, which keeps the build rules for various compiler-rt runtimes consistent at every revision: if there's a feature in add_compiler_rt_runtime function they use, then either all of them make use of it, or none of them. In that case the path could be:
1) add simple and short version of add_compiler_rt_runtime and migrage build rules to use it
2) (in a separate change) add PARENT_TARGET argument, and use it where applicable
3) (in a separate change) add LINK_LIBS argument, and use it where applicable.
4) (in a separate change) add multi-arch support and use it
...

It could make the changes smaller and more granular, and (as mentioned earlier) the code would be consistent at every point. On a flip side, each change would touch multiple files across a lot of directories. It's a matter of taste, really, and it's probably not worth to split this change into several smaller ones at this point. I'll be happy either way as long as you will lay out the plan for further refactoring in a commit message.


http://reviews.llvm.org/D12292