[libcxx-commits] [PATCH] D99178: [libcxx] Disable c++experimental by default in DLL builds

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/docs/BuildingLibcxx.rst:93
           -DLIBCXX_ENABLE_STATIC=NO               ^
-          -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=NO ^
           \path\to\libcxx
----------------
mstorsjo wrote:
> Mordante wrote:
> > I assume things go wrong if the user uses `-DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=YES`. Can you document this isn't supported? I think it would be nice if CMake doesn't allow this invalid combination.
> I kind of wanted to leave it possible to do that still, while you'd essentially be on your own in that case. But I guess I could at least add a cmake warning for that combination - or a hard error if others also feel that way.
I'm mainly concerned with the "removing" of documentation. Before the documentation mentioned you shouldn't do `-DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=YES`. After the patch is says nothing and using this will cause errors.  So I think it's a good thing CMake does the right thing by default, but I dislike the removal of clarity.

I think we should add some documentation here the combination isn't supported.

In my personal opinion I think we should add a hard error in CMake, something like:
```
if (WIN32 AND LIBCXX_ENABLE_SHARED AND LIBCXX_ENABLE_EXPERIMENTAL_LIBRARY)
    message(fatal_error "The libc++experimental isn't available for Windows shared libraries")
endif()
```
But if you rather show a warning I won't object.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99178