[PATCH] D14561: Add CMakeLists for External/{Nurbs|Povray|skidmarks10|HMMER}

[Chris Bieneman via llvm-commits]

beanz added inline comments.

================
Comment at: CMakeLists.txt:59
@@ +58,3 @@
+  if(CMAKE_SIZEOF_VOID_P EQUAL 8)
+    set(ARCH_LP64 TRUE CACHE BOOL "Whether target architecture has 64bit pointers")
+  endif()
----------------
jmolloy wrote:
> Don't we want something like this instead?:
> 
>   if(CMAKE_SIZEOF_VOID_P EQUAL 8)
>     set(TMP_LP64 TRUE)
>   else()
>     set(TMP_LP64 FALSE)
>   endif()
>   set(ARCH_LP64 ${TMP_LP64} CACHE BOOL "Does the target architecture has 64-bit pointers?")
> 
> I'll defer to Chris B on this
Why does this need to be cached at all?

I generally prefer not using cached variables unless there is a really good reason because CMake has no cache invalidation.

================
Comment at: CMakeLists.txt:73
@@ -65,1 +72,3 @@
 add_subdirectory(MultiSource)
+if(EXTERNALS_DIR)
+	add_subdirectory(External)
----------------
jmolloy wrote:
> I'd prefer if we had a "LLVM_" prefix on all variables we expect to be set by the user. Can we stick to that, like in the other projects' builds?
Rather than LLVM, I would suggest prefixing with TEST_SUITE.

I think each project should have its own prefix. Also, with r252747 variables that start with TEST_SUITE will automatically be passed from the top-level CMake invocation into the sub-project configuration.

================
Comment at: External/CMakeLists.txt:1
@@ +1,1 @@
+llvm_add_subdirectories(HMMER Nurbs Povray skidmarks10)
----------------
jmolloy wrote:
> I think this relies on a CMake module from LLVM, not from test-suite, so it'll fail unless we're in /projects.
> 
> Just doing "add_subdirectory()" several times isn't much harder.
llvm_add_subdirectories actually comes from test-suite, so this is fine.


http://reviews.llvm.org/D14561