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

[NAKAMURA Takumi via llvm-commits]

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