This is an archive of the discontinued LLVM Phabricator instance.

[libc++][format] Removes the experimental status.
ClosedPublic

Authored by Mordante on May 17 2023, 10:40 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rGdff62f5251f2: [libc++][format] Removes the experimental status.
Summary

The code has been quite ready for a while now and there are no more ABI
breaking papers. So this is a good time to mark the feature as stable.

Diff Detail

Event Timeline

Mordante created this revision.May 17 2023, 10:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2023, 10:40 AM
Mordante updated this revision to Diff 523350.May 18 2023, 5:22 AM

CI fixes.

Mordante published this revision for review.May 18 2023, 10:12 AM
Mordante added a subscriber: ldionne.
Mordante added inline comments.
libcxx/test/libcxx/experimental/fexperimental-library.compile.pass.cpp
30

@ldionne you added this file, do we want to keep it? If so we probably need to generated something. I don't think there is a similar PSTL test.

Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2023, 10:12 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.May 23 2023, 9:31 AM
ldionne added a subscriber: philnik.

This LGTM w/ comments.

Thanks a lot for all the work you've put into taking std::format from nothing to a stable feature, this is awesome!

libcxx/test/libcxx/experimental/fexperimental-library.compile.pass.cpp
26–30

This should be removed in this patch.

30

@philnik We should add a test for the PSTL here, that's an oversight. This would make sure that the PSTL is indeed enabled when we pass -fexperimental-library.

Then it makes sense to remove _LIBCPP_HAS_NO_INCOMPLETE_FORMAT and this test still has a purpose. I think that we are likely to have at least one experimental feature in flight at all times, so this test would stay relevant.

This revision is now accepted and ready to land.May 23 2023, 9:31 AM
Mordante updated this revision to Diff 524777.May 23 2023, 9:56 AM
Mordante marked an inline comment as done.

Rebased and addresses review comments.

This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Jun 27 2023, 1:24 AM

Sharing in case anyone else runs into the same problem: This caused Chromium builds to fail on Windows due to not linking against compiler-rt's builtins library. It turns out this patch caused us to compile some code doing 128-bit arithmetic and calling the __udivti3 runtime function. We worked around it by defining _LIBCPP_HAS_NO_INT128 until we can make the builtins library part of all our builds.

Sharing in case anyone else runs into the same problem: This caused Chromium builds to fail on Windows due to not linking against compiler-rt's builtins library. It turns out this patch caused us to compile some code doing 128-bit arithmetic and calling the __udivti3 runtime function. We worked around it by defining _LIBCPP_HAS_NO_INT128 until we can make the builtins library part of all our builds.

Interesting that this caused it. I expect the real cause is std::to_char for 128-bit values, there divisions are used. Does that mean the flag in __config is not set correctly? It was recently updated in D134912.

hans added a comment.Jun 28 2023, 1:22 AM

Does that mean the flag in __config is not set correctly? It was recently updated in D134912.

It's tricky. As discussed on that change, Clang does support 128-bit operations also on Windows so the flag is correct in that sense, but it requires linking with the builtins library which we don't do (yet).

Does that mean the flag in __config is not set correctly? It was recently updated in D134912.

It's tricky. As discussed on that change, Clang does support 128-bit operations also on Windows so the flag is correct in that sense, but it requires linking with the builtins library which we don't do (yet).

I see. Is there an estimate how long it takes to get the linking with the builtins library will take. I wonder whether we should ship libc++17 with 128 bit support on Windows when it may lead to link time error.

hans added a comment.Jul 3 2023, 6:39 AM

Does that mean the flag in __config is not set correctly? It was recently updated in D134912.

It's tricky. As discussed on that change, Clang does support 128-bit operations also on Windows so the flag is correct in that sense, but it requires linking with the builtins library which we don't do (yet).

I see. Is there an estimate how long it takes to get the linking with the builtins library will take. I wonder whether we should ship libc++17 with 128 bit support on Windows when it may lead to link time error.

I think there's a good chance of it happening this year, but probably not before the 17 branch which I'm guessing should happen pretty soon.