[PATCH] D13842: [CMake] Get rid of LLVM_DYLIB_EXPORT_ALL, and make it the default, add libLLVM-C on darwin to cover the C API needs.

[Andrew Wilkins via llvm-commits]

axw added a comment.

Thanks, just a few small things. I'm not qualified to review the Apple-specific bits.


================
Comment at: CMakeLists.txt:376
@@ -375,3 +375,3 @@
 endif()
-option(LLVM_LINK_LLVM_DYLIB "Link tools against the libllvm dynamic library" OFF)
+option(LLVM_LINK_LLVM_DYLIB "Link tools against the libllvm dynamic library" ON)
 option(LLVM_BUILD_LLVM_DYLIB "Build libllvm dynamic library" ${LLVM_LINK_LLVM_DYLIB})
----------------
Did you mean to make this change? It's a pretty big change, and probably warrants some discussion on llvm-dev.

================
Comment at: CMakeLists.txt:378
@@ -377,3 +377,3 @@
 option(LLVM_BUILD_LLVM_DYLIB "Build libllvm dynamic library" ${LLVM_LINK_LLVM_DYLIB})
-option(LLVM_DYLIB_EXPORT_ALL "Export all symbols from libLLVM.dylib (default is C API only" ${LLVM_LINK_LLVM_DYLIB})
+option(LLVM_BUILD_LLVM_C_DYLIB "Build libllvm dynamic library" OFF)
 set(LLVM_DISABLE_LLVM_DYLIB_ATEXIT_DEFAULT ON)
----------------
nit, differentiate description from option above?

================
Comment at: tools/llvm-shlib/CMakeLists.txt:15
@@ -16,1 +14,3 @@
+  if(LLVM_DYLIB_EXPORTED_SYMBOL_FILE)
+    message(WARNING "Using LLVM_DYLIB_EXPORT_ALL with LLVM_DYLIB_EXPORTED_SYMBOL_FILE may not work. Use at your own risk.")
   endif()
----------------
s/LLVM_DYLIB_EXPORT_ALL/LLVM_LINK_LLVM_DYLIB/
?

================
Comment at: tools/llvm-shlib/CMakeLists.txt:80
@@ +79,3 @@
+  add_custom_command(OUTPUT ${LLVM_EXPORTED_SYMBOL_FILE}
+    COMMAND nm ${LIB_PATH} | awk "/T _LLVM/ || /T LLVM/ { print $3 }" | sort -u | sed -e "s/^_//g" > ${LLVM_EXPORTED_SYMBOL_FILE}
+    WORKING_DIRECTORY ${LIB_DIR}
----------------
nit, I think "_LLVM || LLVM" is only necessary if targeting Darwin (with a _ prefix) as well as other platforms (with no _ prefix).


http://reviews.llvm.org/D13842