<div dir="ltr">> I can change the layout to this to give more of a "one-item in each line" feeling which should be good for subdirectories.<div><br></div><div>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.</div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, 11 Nov 2015 at 17:43 Matthias Braun via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">MatzeB added inline comments.<br>
<br>
================<br>
Comment at: CMakeLists.txt:59<br>
@@ +58,3 @@<br>
+  if(CMAKE_SIZEOF_VOID_P EQUAL 8)<br>
+    set(ARCH_LP64 TRUE CACHE BOOL "Whether target architecture has 64bit pointers")<br>
+  endif()<br>
----------------<br>
beanz wrote:<br>
> jmolloy wrote:<br>
> > Don't we want something like this instead?:<br>
> ><br>
> >   if(CMAKE_SIZEOF_VOID_P EQUAL 8)<br>
> >     set(TMP_LP64 TRUE)<br>
> >   else()<br>
> >     set(TMP_LP64 FALSE)<br>
> >   endif()<br>
> >   set(ARCH_LP64 ${TMP_LP64} CACHE BOOL "Does the target architecture has 64-bit pointers?")<br>
> ><br>
> > I'll defer to Chris B on this<br>
> Why does this need to be cached at all?<br>
><br>
> I generally prefer not using cached variables unless there is a really good reason because CMake has no cache invalidation.<br>
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.<br>
<br>
Anyway I agree that in this specific instance it's probably best to not make it cached at all.<br>
<br>
================<br>
Comment at: CMakeLists.txt:73<br>
@@ -65,1 +72,3 @@<br>
 add_subdirectory(MultiSource)<br>
+if(EXTERNALS_DIR)<br>
+       add_subdirectory(External)<br>
----------------<br>
beanz wrote:<br>
> jmolloy wrote:<br>
> > 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?<br>
> Rather than LLVM, I would suggest prefixing with TEST_SUITE.<br>
><br>
> 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.<br>
TEST_SUITE_ prefix sounds good.<br>
<br>
================<br>
Comment at: External/CMakeLists.txt:1<br>
@@ +1,1 @@<br>
+llvm_add_subdirectories(HMMER Nurbs Povray skidmarks10)<br>
----------------<br>
beanz wrote:<br>
> jmolloy wrote:<br>
> > I think this relies on a CMake module from LLVM, not from test-suite, so it'll fail unless we're in /projects.<br>
> ><br>
> > Just doing "add_subdirectory()" several times isn't much harder.<br>
> llvm_add_subdirectories actually comes from test-suite, so this is fine.<br>
I can change the layout to this to give more of a "one-item in each line" feeling which should be good for subdirectories.<br>
<br>
```<br>
llvm_add_subdirectories(<br>
  HMMER<br>
  Nurbs<br>
  Povray<br>
  skidmarks10<br>
)<br>
```<br>
<br>
<br>
<br>
<a href="http://reviews.llvm.org/D14561" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14561</a><br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>