[PATCH] D26652: [CMake] Multi-target builtins build

[Chris Bieneman via llvm-commits]

beanz added a subscriber: rengolin.
beanz added a comment.

Minor comments below. Otherwise LGTM! Thank you for picking this up. I'm really excited to see this taking shape.

Looping in @rengolin because he has been interested in this in the past.



================
Comment at: cmake/modules/LLVMExternalProjectUtils.cmake:47
   if(NOT ARG_TOOLCHAIN_TOOLS)
-    set(ARG_TOOLCHAIN_TOOLS clang lld)
+    set(ARG_TOOLCHAIN_TOOLS clang lld llvm-ar)
   endif()
----------------
Adding llvm-ar to the default toolchain tools isn't valid on all platforms. In fact, I only think it is valid for Linux-based platforms, because it isn't for Darwin or (I think) Windows. Can you make this conditional?


================
Comment at: cmake/modules/LLVMExternalProjectUtils.cmake:74
 
+  list(FIND TOOLCHAIN_TOOLS llvm-ar FOUND_ARCHIVER)
+  if(FOUND_ARCHIVER GREATER -1)
----------------
CMake now has `if(... IN_LIST ...)` that makes this simpler. I know we're not universally using it yet, but we should at least use it in new code because it is way easier to read.


Repository:
  rL LLVM

https://reviews.llvm.org/D26652