<div class="gmail_quote">On Thu, Dec 29, 2011 at 8:09 AM, arrowdodger <span dir="ltr"><<a href="mailto:6yearold@gmail.com">6yearold@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Updated patches attached. Changes:<br>1. All generating targets are now running only when user is invoking install rule. Credits go to @sakra on SO.<br>2. Wrapped some common code into macros, put them into cmake/modules/AddLLVM.cmake.<br>


3. Create .tar.gz's only if both tar and gzip are found, otherwise warn user and don't even generate targets.<br>4. Code style fixes.</blockquote><div><br></div><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><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><br></div><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><br></div><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'. 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. I also have trouble believing CMake can't represent this pattern, but I haven't tried my hand at it...</div>
</div>