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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

LGTM modulo a small suggestion. If you don't want to address that suggestion, please rebase to trigger a CI run. I'd like to see it builds without issues before giving my approval.



================
Comment at: libcxx/CMakeLists.txt:416
+                      " static library, while the headers signal dllimport"
+                      " linkage.")
+endif()
----------------
Nice. One suggestion "Windows, as the" -> "Windows. The".


================
Comment at: libcxx/docs/BuildingLibcxx.rst:93
           -DLIBCXX_ENABLE_STATIC=NO               ^
-          -DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=NO ^
           \path\to\libcxx
----------------
mstorsjo wrote:
> Mordante wrote:
> > 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.
> I'd rather keep such details out of this part of the docs - there's still some amount of ifs and buts around this configuration, so it's better if we can reduce some amount of the noise in it at least, But I guess I can make it an error on the cmake level - that would at least make the situation clearer.
Oke fair enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99178