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

[James Molloy via llvm-commits]

> I can change the layout to this to give more of a "one-item in each line"
feeling which should be good for subdirectories.

I'm not fussed about that; I was only concerned we were relying on
something not always available. As Chris has said this isn't the case, it's
fine and you can ignore my comment.

On Wed, 11 Nov 2015 at 17:43 Matthias Braun via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> MatzeB 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()
> ----------------
> beanz wrote:
> > 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.
> My goal here was not so much the caching but more about giving the user
> control over these settings. Though I guess the proper design would be to
> only give a variable to the user to override the auto detection instead of
> saving the result of the auto detection in the cache.
>
> Anyway I agree that in this specific instance it's probably best to not
> make it cached at all.
>
> ================
> Comment at: CMakeLists.txt:73
> @@ -65,1 +72,3 @@
>  add_subdirectory(MultiSource)
> +if(EXTERNALS_DIR)
> +       add_subdirectory(External)
> ----------------
> beanz wrote:
> > 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.
> TEST_SUITE_ prefix sounds good.
>
> ================
> Comment at: External/CMakeLists.txt:1
> @@ +1,1 @@
> +llvm_add_subdirectories(HMMER Nurbs Povray skidmarks10)
> ----------------
> beanz wrote:
> > 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.
> I can change the layout to this to give more of a "one-item in each line"
> feeling which should be good for subdirectories.
>
> ```
> llvm_add_subdirectories(
>   HMMER
>   Nurbs
>   Povray
>   skidmarks10
> )
> ```
>
>
>
> http://reviews.llvm.org/D14561
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151111/c92b8e34/attachment.html>