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

Tue Jun 13 07:02:52 PDT 2023

mstorsjo added a comment.

In D144734#4417378 <https://reviews.llvm.org/D144734#4417378>, @andrewng wrote:

> It should be easy to split out some of the miscellaneous changes that are related but not directly dependent. Some of the other changes might not be so straightforward. My main concern is time, splitting it up will need more time and testing. The testing can be particularly time and resource consuming. I kind of added `LIBCXX_ENABLE_EXPERIMENTAL_DLL`, which is off by default, because of its "controversial" nature.

Right - but even if it's off by default, committing it requires general approval - the libcxx maintainers (not including me) have gone a fair bit in the other direction of removing such configurability to reduce the number of potential configurations, so adding new options, even if disabled, can also be controversial.

>> Regarding ways of selecting the CRT, unfortunately things aren't quite straightforward yet - even if the minimum of CMake 3.20 was committed, the behaviour of the new policy CMP0091, https://cmake.org/cmake/help/latest/policy/CMP0091.html, was reverted to the old one in D150688 <https://reviews.llvm.org/D150688> - see https://github.com/llvm/llvm-project/issues/63286. So for now, the old mechanism is defacto in place still. I don't quite know how setting the `MSVC_RUNTIME_LIBRARY` property on a target behaves though. Instead of intercepting the individual `/MT` flags, I guess we could just look at the `LLVM_USE_CRT_*` flags from ChooseMSVCCRT.cmake (https://github.com/llvm/llvm-project/blob/llvmorg-16.0.3/llvm/cmake/modules/ChooseMSVCCRT.cmake) though.
>
> Yes, this is a bit of a mess right now. I basically take the approach of setting both `MSVC_RUNTIME_LIBRARY` and `LLVM_USE_CRT_*` to my preferred runtime and that appears to work for now.

Ok, I see.

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

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144734