[libcxx-commits] [PATCH] D128928: [libc++] Treat incomplete features just like other experimental features

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 3 inline comments as done.
ldionne added a subscriber: jwakely.
ldionne added a comment.

In D128928#3655204 <https://reviews.llvm.org/D128928#3655204>, @Mordante wrote:

> In general I'm happy, but I see a lot of bootstrap CI failures. Are they due to this patch or unrelated.

They are unrelated, and in fact they have been fixed in trunk (by reverting a Clang change).



================
Comment at: libcxx/benchmarks/CMakeLists.txt:126
   endif()
-  if (TARGET cxx_experimental)
-    target_link_libraries(${libcxx_target} PRIVATE cxx_experimental)
-  endif()
-  target_link_libraries(${libcxx_target} PRIVATE -lbenchmark)
+  target_link_libraries(${libcxx_target} PRIVATE cxx_experimental -lbenchmark)
   if (LLVM_USE_SANITIZER)
----------------
Mordante wrote:
> Is this change really needed, should we instead set `-fexperimental-libray` unconditionally.
> 
> Drive-by why `-lbenchmark` instead of `benchmark`?
Not all compilers support `-fexperimental-library`, so I think we're better off linking against `cxx_experimental` directly. It also ensures that we're using the `libc++experimental.a` that we just built, as opposed to some other experimental library that might be supported by the compiler.


================
Comment at: libcxx/docs/Status/Cxx20.rst:37
 .. csv-table::
    :file: Cxx20Papers.csv
    :header-rows: 1
----------------
Mordante wrote:
> ldionne wrote:
> > TODO: Add a release note explaining what `-funstable` does.
> This is still open.
I will actually do that in D121141, since this patch now has no mentions of `-fexperimental-library`. I wanted to make sure that both changes make sense in isolation, which is why I've removed all references to a compiler flag in this patch (and I'm using `-D_LIBCPP_ENABLE_EXPERIMENTAL` instead.


================
Comment at: libcxx/src/CMakeLists.txt:336
 cxx_add_common_build_flags(cxx_experimental)
+target_compile_options(cxx_experimental PUBLIC -D_LIBCPP_ENABLE_EXPERIMENTAL) # TODO: Replace this by a proper compiler option once supported
 
----------------
Mordante wrote:
> If possible I'd like to see this solved before landing since D121141 has been approved.
I think the TODO actually shouldn't exist. Indeed, after discussing with @jwakely on Discourse, it seems like this is going to be required for GCC users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128928