[libcxx-commits] [PATCH] D144734: [libcxx] Enable support for static and debug Windows runtimes

[Mark de Wever via Phabricator via libcxx-commits]

Mordante requested changes to this revision.
Mordante added a comment.
This revision now requires changes to proceed.

In D144734#4417496 <https://reviews.llvm.org/D144734#4417496>, @mstorsjo wrote:

> In D144734#4417378 <https://reviews.llvm.org/D144734#4417378>, @andrewng wrote:
>
>> I will try to split up this patch but as it's now CI "friendly", it would be great if it doesn't need to be split up too much. There's quite a lot going on but it's also not too large a patch either.
>
> While it's not super large, it has way too many orthogonal nontrivial changes, so it definitely does need to be split up.

I like that people try to be CI "friendly". But I consider a clean git history more important. When a patch breaks something on a platform we revert the entire patch, so when 3/4 of the changes are an improvement 4/4 will be reverted.

>> If `LIBCXX_ENABLE_EXPERIMENTAL_DLL` is too controversial, it can always be removed later?
>
> While I'm not a libcxx maintainer, I would say that that's not really the way it works - I don't think we merge things "just in case", but what's merged are things that are fully agreed on first.

As mentioned in the comments this should really be a different patch and only be added if it's useful. We indeed don't add features that might be useful, we want features that are useful to (a part of) our user base.

As mentioned by @mstorsjo we have enabled 128-bit support on Windows now, please update your patch for these changes.



================
Comment at: libcxx/CMakeLists.txt:755-758
+    target_link_libraries(${target} PRIVATE ${UCRT_LIB}${LIB_SUFFIX}) # Universal C runtime
+    target_link_libraries(${target} PRIVATE ${CXXRT_LIB}${LIB_SUFFIX}) # C++ runtime
+    target_link_libraries(${target} PRIVATE ${CRT_LIB}${LIB_SUFFIX}) # C runtime startup files
+    target_link_libraries(${target} PRIVATE ${CXX_LIB}${LIB_SUFFIX}) # C++ standard library. Required for exception_ptr internals.
----------------
Why not one `target_link_libraries` instead? I would like to keep one library + comment per line.


================
Comment at: libcxx/src/CMakeLists.txt:270
   endif()
+  if(LIBCXX_TARGETING_MSVC AND LIBCXX_ENABLE_EXPERIMENTAL_DLL)
+    target_link_libraries(cxx_shared PRIVATE cxx_experimental_stub)
----------------
I'm too quite wary of this flag, especially since there are no CI changes to test this. Most (all?) libc++ developers are not on Windows, so without a CI to test this it will break. I really want to see this in a separate patch.


================
Comment at: libcxx/test/support/set_windows_crt_report_mode.h:12-22
 
-#ifndef _DEBUG
-#error _DEBUG must be defined when using this header
-#endif
+#if defined(__cplusplus) && __cplusplus > 199711L
+#  ifndef _DEBUG
+#    error _DEBUG must be defined when using this header
+#  endif
 
+#  ifndef _WIN32
----------------
Can you undo these unrelated formatting changes?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144734/new/

https://reviews.llvm.org/D144734