[PATCH] D12864: [CMake] Refactor and cleanup generating and installing symlinks to tools.

[Chris Bieneman via llvm-commits]

I agree. I started looking at that this afternoon. I should have patches tomorrow.

-Chris

> On Sep 14, 2015, at 5:16 PM, NAKAMURA Takumi <geek4civic at gmail.com> wrote:
> 
> chapuni added a comment.
> 
> Chris, do you think add_llvm_tool_symlink may be applicable just to llvm-ar?
> I think also clang's cmakefiles may be cleaned up.
> 
> 
> ================
> Comment at: llvm/trunk/cmake/modules/AddLLVM.cmake:1026
> @@ +1025,3 @@
> +
> +function(add_llvm_tool_symlink name target)
> +  if(UNIX)
> ----------------
> I suggest we may reconsider the term "target" around here.
> It handles both "the target of add_executable" and "the target of add_custom_target below". It may confuse us.
> 
>  - CMake uses "target" as each target command.
>  - Userland command, like cp(1), ln(1) uses "target" as destination.
> 
> "dest" may be appropriate, IMO.
> 
> ================
> Comment at: llvm/trunk/cmake/modules/AddLLVM.cmake:1038
> @@ +1037,3 @@
> +  add_custom_command(OUTPUT ${output_path}
> +                     COMMAND ${CMAKE_COMMAND} -E ${LLVM_LINK_OR_COPY} "${target_binary}" "${output_path}"
> +                     DEPENDS ${target})
> ----------------
> I suggest lower indentation with two spaces, like;
> ```
>  add_custom_command(OUTPUT ${output_path}
>    COMMAND ${CMAKE_COMMAND} -E ${LLVM_LINK_OR_COPY} "${target_binary}" "${output_path}"
> ```
> Also emacs' cmake-mode suggests so.
> 
> ================
> Comment at: llvm/trunk/cmake/modules/AddLLVM.cmake:1044
> @@ +1043,3 @@
> +
> +  # MAke sure the parent tool is a toolchain tool, otherwise exclude this tool
> +  list(FIND LLVM_TOOLCHAIN_TOOLS ${target} LLVM_IS_${target}_TOOLCHAIN_TOOL)
> ----------------
> typo.
> 
> 
> Repository:
>  rL LLVM
> 
> http://reviews.llvm.org/D12864
> 
> 
>