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

Wed Jun 14 09:52:18 PDT 2023

andrewng added a comment.

>> 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.

Yes, a clean git history is important but so is getting better platform test coverage. It's always somewhat of a balancing act in the end.

>>> 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.

The reason for the `c++experimental` DLL is that this is the simplest way to enable `c++experimental` testing when using the MSVC DLL run-time. There are alternative solutions which would involve using different "DLL" annotations for components of `c++experimental` but I believe this would be more of a maintenance burden/annoyance, particularly for non-Windows users!

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

Yes, IIUC, `int128` support has just been disabled for `clang-cl`. My intention with this patch was to enable `int128` testing for `clang-cl`/MSVC despite all the issues regarding proper deployment of this feature. However, it seems that the preferred approach is to remove all `int128` related support.

================
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.
----------------
Mordante wrote:
> Why not one `target_link_libraries` instead? I would like to keep one library + comment per line.
Yes, definitely could be merged into one. I was just following the existing style on the assumption that it was the "preferred" style.

================
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)
----------------
Mordante wrote:
> 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.
IIRC, @mstorsjo suggested adding the "new" CI configurations as a separate patch. However, if I'm going to be splitting things up then I may be able to add CI configurations as appropriate. Are there any guidelines regarding how many CI configurations can/should be added? Also debug based configurations can be extremely slow!

================
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
----------------
Mordante wrote:
> Can you undo these unrelated formatting changes?
I'm pretty sure this was `clang-format`'s doing, but yes when I split this patch out, I can remove the `clang-format` changes if that's preferred.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144734