[PATCH] D13059: [CMake] [Darwin] Bug 21562 - Add a CMake equivalent for make/platform/clang_darwin.mk in compiler_rt

[Alexey Samsonov via llvm-commits]

samsonov added inline comments.

================
Comment at: cmake/Modules/CompilerRTDarwinUtils.cmake:107
@@ +106,3 @@
+  if(EXISTS ${CMAKE_SOURCE_DIR}/lib/builtins/Darwin-excludes/${os}.txt)
+    file(READ ${CMAKE_SOURCE_DIR}/lib/builtins/Darwin/${os}.txt ${os}_BUILTINS)
+    string(REPLACE "\n" ";" ${os}_BUILTINS ${${os}_BUILTINS})
----------------
Please fix directory name to Darwin-excludes here and below. Or, better, hoist the directory name to a variable.
  set(DARWIN_EXCLUDES_DIR ${COMPILER_RT_SOURCE_DIR}/lib/builtins/Darwin-excludes)
and use it from then on.

================
Comment at: cmake/Modules/CompilerRTDarwinUtils.cmake:119
@@ +118,3 @@
+    string(REGEX MATCH "${os}([0-9\\.]*)-${arch}.txt" VERSION_MATCHED "${builtin_list}")
+    if(VERSION_MATCHED)
+      if(CMAKE_MATCH_1 VERSION_EQUAL min_version OR CMAKE_MATCH_1 VERSION_GREATER min_version)
----------------
  if (VERSION_MATCHED AND NOT CMAKE_MATCH_1 VERSION_LESS min_version)
?

================
Comment at: cmake/Modules/CompilerRTDarwinUtils.cmake:123
@@ +122,3 @@
+        string(REPLACE "\n" ";" ${arch}_${os}_BUILTINS ${${arch}_${os}_BUILTINS})
+        set(${arch}_${os}_EXCLUDED_BUILTINS
+          ${${arch}_${os}_BUILTINS}
----------------
I don't understand the logic in this function:
  * if I happened to have ios.txt, but not ios6-armv7.txt, this statement will never be executed, and ${arch}_${os}_EXCLUDED_BUILTINS will be empty.
  * if I have ios6-armv7.txt and ios7-armv7.txt, and I configure for armv7 and min_version=6.0, I will set the contents of ${arch}_${os}_EXCLUDED_BUILTINS twice to different values, that would depend on the order of files in glob.

Neither of this look correct to me.


http://reviews.llvm.org/D13059