[PATCH] D28441: [libc++] [CMake] Link with /nodefaultlibs on Windows

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added inline comments.


================
Comment at: CMakeLists.txt:43
+if (WIN32 AND NOT MINGW)
+  set(LIBCXX_TARGETING_WINDOWS ON)
+else()
----------------
Not the biggest fan of this name, since it's not obvious why MinGW shouldn't count as targeting Windows. I thought of `LIBCXX_TARGETING_NATIVE_WINDOWS` or `LIBCXX_TARGETING_MSVCRT` instead, but MinGW is also native Windows and targets MSVCRT, so those names aren't any better from that perspective either. I can't think of anything else at the moment, but I'm sure there's a better name.


================
Comment at: CMakeLists.txt:388
+# non-debug DLLs
+remove_flags("/D_DEBUG" "/MTd" "/MDd" "/MT" "/Md" "/RTC1")
+
----------------
cl (and presumably clang-cl) also accepts all these flags with a leading dash instead of a leading slash, and cmake at least has a tendency to do `-D` instead of `/D`, so you might need to take those into account as well. Also, what about the other potential `/RTC` options?


================
Comment at: lib/CMakeLists.txt:108-109
+if (LIBCXX_TARGETING_WINDOWS)
+  add_compile_flags(/Zl)
+  add_link_flags(/nodefaultlib)
+  add_library_flags(ucrt) # Universal C runtime
----------------
These should be guarded under a check for a cl or cl-like frontend rather than `LIBCXX_TARGETING_WINDOWS` (since in theory we could be using the regular clang frontend to compile for Windows as well).


================
Comment at: lib/CMakeLists.txt:111
+  add_library_flags(ucrt) # Universal C runtime
+  add_library_flags(vcruntime) # C++ runtime
+  add_library_flags(msvcrt) # C runtime startup files
----------------
Idk if there's anything specific to C++ in vcruntime; it's more compiler runtime functions as far as I know.


================
Comment at: lib/CMakeLists.txt:113
+  add_library_flags(msvcrt) # C runtime startup files
+  add_library_flags(iso_stdio_wide_specifiers)
+endif()
----------------
Maybe add a comment explaining the purpose of this one as well?


https://reviews.llvm.org/D28441