[PATCH] D39814: Create a Cross-Compilation CMake toolchain file for NonWindows -> Windows

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

Thanks for productionizing this! It'll be awesome to have it in-tree.



================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:13
+# LLVM_NATIVE_TOOLCHAIN:
+#   *Absolute path* a folder containing the toolchain which will be used to
+#   build.  At a minimum, this folder should have a bin directory with a
----------------
**to** a folder


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:29
+#   lib
+#     x64
+#       libcmt.lib
----------------
x64 is an MSVC 2017 thing; 2015 used amd64. I think it's perfectly fine to only care about 2017, but it's probably worth noting. (People using 2015 can just create a symlink or rename their folder or something.)


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:89
+
+function(init_user_prop prop)
+  if (${prop})
----------------
I'd add an explanatory comment about why you need to do this seemingly weird thing.


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:90
+function(init_user_prop prop)
+  if (${prop})
+    set(ENV{_${prop}} "${${prop}}")
----------------
This is the nittiest of all nits, but it seems to be more common in LLVM's cmake to not have spaces after `if`, `foreach`, etc. I like having the space personally, but when in Rome ...


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:104
+# a different driver name.  User can override this by manually specifying
+# -DCROSS_TOOLCHAIN_FLAGS_NATIVE
+list(APPEND _CTF_NATIVE_DEFAULT "-DCMAKE_ASM_COMPILER=${LLVM_NATIVE_TOOLCHAIN}/bin/clang")
----------------
I think a brief comment about why this is required would be useful.


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:121
+    NOT EXISTS "${LLVM_NATIVE_TOOLCHAIN}/bin/lld-link")
+  message(FATAL_ERROR
+          "LLVM_NATIVE_TOOLCHAIN folder '${LLVM_NATIVE_TOOLCHAIN}' does not "
----------------
In general I prefer `SEND_ERROR`, so that all errors are output at once, whereas with `FATAL_ERROR` you have to whack them one at a time.


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:131
+
+if (NOT EXISTS "${MSVC_BASE}" OR
+    NOT EXISTS "${MSVC_INCLUDE}" OR
----------------
Nit: this conditional should be moved up to where the `MSVC_INCLUDE` and `MSVC_LIB` variables are set, or those should be moved down here.


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:153
+    # FIXME: We should find a way to automatically detect the version here.
+    -Xclang -isystem -Xclang "${LLVM_NATIVE_TOOLCHAIN}/lib/clang/6.0.0/include"
+    -Xclang -isystem -Xclang "${MSVC_INCLUDE}"
----------------
Serus on IRC introduced me to the existence of `-imsvc`, which should be equivalent but is more concise.

The other big advantage of `-imsvc` is that we could make those directories be ordered after the resource dir (if they aren't already), which would save us from having to write out the resource dir manually. I think in general it's more correct for `-imsvc` to be ordered after the resource dir anyway.


================
Comment at: llvm/cmake/platforms/WinMsvc.cmake:167
+# only be populated on the initial configure, and their values won't change
+# afterward. If you don't care about preserving the initial user flags, you can
+# just set CMAKE_C_FLAGS and friends directly with FORCE and skip this whole
----------------
You can get rid of the "If you don't care about preserving ..." part, since if we're doing this in LLVM we definitely care.


https://reviews.llvm.org/D39814