Page MenuHomePhabricator

[libc++] Treat incomplete features just like other experimental features
ClosedPublic

Authored by ldionne on Jun 30 2022, 9:20 AM.

Details

Summary

In particular remove the ability to expel incomplete features from the
library at configure-time, since this can now be done through the
-funstable compiler flag.

Also, never provide symbols related to incomplete features inside the
dylib, instead provide them in c++experimental.a (this changes the
symbols list, but not for any configuration that should have shipped).

Diff Detail

Event Timeline

ldionne created this revision.Jun 30 2022, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 9:20 AM
ldionne requested review of this revision.Jun 30 2022, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 9:20 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante added a comment.EditedJun 30 2022, 9:46 AM

In general I'm happy with this patch. I only wonder whether or not all tests will pass with these changes. So I want to have a second look after a CI run.

/edit removed parent revision comment since that was done at the same time I was reviewing the patch.

ldionne added inline comments.Jul 5 2022, 12:47 PM
libcxx/docs/Status/Cxx20.rst
37

TODO: Add a release note explaining what -funstable does.

ldionne updated this revision to Diff 443347.Jul 8 2022, 2:03 PM

Rebase onto main and remove references to -funstable.

ldionne updated this revision to Diff 444656.Thu, Jul 14, 7:36 AM

Rebase, fix some CI issues.

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

libcxx/benchmarks/CMakeLists.txt
126

Is this change really needed, should we instead set -fexperimental-libray unconditionally.

Drive-by why -lbenchmark instead of benchmark?

libcxx/docs/Status/Cxx20.rst
37

This is still open.

libcxx/src/CMakeLists.txt
337

If possible I'd like to see this solved before landing since D121141 has been approved.

ldionne marked 3 inline comments as done.Fri, Jul 15, 9:55 AM
ldionne added a subscriber: jwakely.

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

libcxx/benchmarks/CMakeLists.txt
126

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.

libcxx/docs/Status/Cxx20.rst
37

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.

libcxx/src/CMakeLists.txt
337

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.

ldionne updated this revision to Diff 445037.Fri, Jul 15, 9:55 AM
ldionne marked 3 inline comments as done.

Rebase, remove some TODOs about _LIBCPP_ENABLE_EXPERIMENTAL

Mordante accepted this revision.Fri, Jul 15, 10:07 AM

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

I saw the message on Discord after reviewing this patch.

Thanks for the update, LGTM!

This revision is now accepted and ready to land.Fri, Jul 15, 10:07 AM