On Mon, Jan 2, 2012 at 9:36 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div class="gmail_quote"><div><div class="h5"><div>Thanks, but there are some real problems with how this is structured. The first two are easy to fix, but the last may not be.</div></div></div>
<div><br></div><div>1) The macros are strangely formed in a few ways. First, please avoid magical arrays filled in with the results of your macro. They're very hard to understand. I'd much rather that each macro produces a new target which other things can then depend on, or populates an array the name of which is passed into the macro. Also, why make the user write a foreach loop around the files? It seems like you could have them provide a list of files rather than a single file, and do the looping inside the macro. Finally, please allow the user to specify the actual filename rather than the basename of the podfile. Compute the basename yourself, as this will make both the use of the macro easier to understand and it's implementation more clear.</div>

</div></blockquote><div><br>Does it looks better now?<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote">
<div>2) Please use something more distinct such as 'llvm-docs-*' as the custom target names; 'llvm-ps' looks too much like it could be one of the actual LLVM tools.</div></div></blockquote><div><br>Done.<br>

 <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><div>3) I don't understand why you can't use normal CMake dependencies to connect the install rules to the actual build steps. It seems like you could use install(FILES ${HTML_FILES} ...) or some equivalent construct. CMake should be handling the dependency computations and ensuring that those files are build for 'make install'. </div>

</div></blockquote><div><br>Yeah, it should. But it does not. I've even asked on StackOverflow about this.<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

<div class="gmail_quote"><div>If CMake cannot do this naturally with its own dependency information, then I fear it may not be worth adding this to the CMake builds, and they will be less functional than the autotools builds. I think re-executing the make tool is an unacceptable design wart.</div>

</div></blockquote><div><br>Why do you think so? Isn't that hack is what CMake should actually do - just call `make <target>` during `make install`? If CMake fix it, this hack can be removed by altering only 3 lines in AddLLVM.cmake.<br>

<br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><div>I also have trouble believing CMake can't represent this pattern, but I haven't tried my hand at it...</div>


</div>
</blockquote></div><br>If you find cleaner way to solve this, i would be happy to redo the patch.<br>