This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Remove availability markup for std::format
ClosedPublic

Authored by ldionne on Sep 25 2022, 2:43 AM.

Details

Reviewers
Mordante
Group Reviewers
Restricted Project
Commits
rG3d334df58742: [libc++] Remove availability markup for std::format
Summary

std::format is currently experimental, so there is technically no
deployment target requirement for it (since the only symbols required
for it are in libc++experimental.a).

However, some parts of std::format depend indirectly on the floating
point std::to_chars implementation, which does have deployment target
requirements.

This patch removes all the availability format for std::format and
updates the XFAILs in the tests to properly explain why they fail
on old deployment targets, when they do. It also changes a couple
of tests to avoid depending on floating-point std::to_chars when
it isn't fundamental to the test.

Finally, some tests are marked as XFAIL but I added a comment saying

TODO FMT This test should not require std::to_chars(floating-point)

These tests do not fundamentally depend on floating-point std::to_chars,
however they end up failing because calling std::format even without a
floating-point argument to format will end up requiring floating-point
std::to_chars. I believe this is an implementation artifact that could
be avoided in all cases where we know the format string at compile-time.
In the tests, I added the TODO comment only to the places where we could
do better and actually avoid relying on floating-point std::to_chars
because we know the format string at compile-time.

Diff Detail

Event Timeline

Mordante created this revision.Sep 25 2022, 2:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2022, 2:43 AM
Mordante updated this revision to Diff 462708.Sep 25 2022, 3:22 AM

Attempts to fix CI.

Mordante updated this revision to Diff 462713.Sep 25 2022, 4:19 AM

Attempts to fix CI.

Mordante updated this revision to Diff 462718.Sep 25 2022, 4:30 AM

Attempts to fix CI.

Mordante updated this revision to Diff 462722.Sep 25 2022, 4:45 AM

Attempt to fix CI.

Mordante updated this revision to Diff 463620.Sep 28 2022, 10:56 AM

CI testing.

Mordante updated this revision to Diff 463633.Sep 28 2022, 11:28 AM

More tests

Mordante updated this revision to Diff 479359.Dec 1 2022, 10:47 AM

Attempt to fix the CI.

Mordante updated this revision to Diff 479928.Dec 4 2022, 9:38 AM

Attempts to fix CI.

Mordante updated this revision to Diff 479934.Dec 4 2022, 10:42 AM

CI fixes.

Mordante updated this revision to Diff 479940.Dec 4 2022, 12:16 PM

Rebased, formatted, and reenable the entire CI.

Mordante updated this revision to Diff 480113.Dec 5 2022, 8:05 AM

CI fixes.

Mordante updated this revision to Diff 480117.Dec 5 2022, 8:23 AM

Fixes CI.

ldionne added a subscriber: ldionne.Dec 6 2022, 9:32 AM
ldionne added inline comments.
libcxx/include/__chrono/formatter.h
52

This really looks like a compiler bug to me. Would you be able to get a smaller reproducer for the issue? We could get it fixed in Clang.

libcxx/test/std/utilities/format/format.functions/vformat_to.locale.pass.cpp
15

I think that should work too, and IMO it's easier to read.

Mordante published this revision for review.Dec 6 2022, 10:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 6 2022, 10:09 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante added inline comments.Feb 6 2023, 10:11 AM
libcxx/include/__chrono/formatter.h
52

I have hard time to get a reproducer, I probably want to investigate it at a later time.

Mordante updated this revision to Diff 495205.Feb 6 2023, 10:18 AM

Rebased and addresses review comments.

Mordante updated this revision to Diff 495433.Feb 7 2023, 2:02 AM

CI fixes.

Mordante updated this revision to Diff 495618.Feb 7 2023, 12:48 PM

CI fixes.

Mordante updated this revision to Diff 495844.Feb 8 2023, 7:27 AM

CI fixes.

ldionne commandeered this revision.Mar 16 2023, 1:45 PM
ldionne added a reviewer: Mordante.

Commandeering per offline discussion.

ldionne updated this revision to Diff 505918.Mar 16 2023, 1:47 PM
ldionne retitled this revision from [libc++][format] Fixes Apple back deployment. to [libc++] Fix availability markup for std::format.
ldionne edited the summary of this revision. (Show Details)

Update with commit message.

ldionne added inline comments.Mar 16 2023, 1:53 PM
libcxx/include/__chrono/formatter.h
44

Most of these changes are removals. The interesting change to see is actually where the annotations were kept, but this diff doesn't show it. As a summary, only the following entities still have the attribute after this change:

operator<<(chrono types) // those use std::format
std::format, std::vformat and everything else in format_functions.h // those use floating-point to_chars indirectly sometimes
formatter<floating-point> in formatter_floating_point.h // those are the only ones that use to_chars directly

All the other _LIBCPP_AVAILABILITY_FORMAT are gone cause they don't use to_chars.

libcxx/include/__format/format_functions.h
442

I want to call this out -- this is terrible but omitting this creates huge problems with the attribute. I am not convinced this is acceptable.

ldionne updated this revision to Diff 506080.Mar 17 2023, 7:30 AM

Add test to check that availability markup triggers appropriately and fix clang-format.

Thanks for picking this up! I have a few questions regarding formatters that could be used with floating-point types. I really wonder whether removing their availability macros is correct.

libcxx/include/__availability
266 ↗(On Diff #506080)

I assume this answers the question of the removal if the macro in classes, right?

libcxx/include/__chrono/ostream.h
95

Depending on the answer regarding the usage of floating-point in general I need to check whether this one can call to_chars(floating-point). The _Rep can be a floating-point type, the same may apply to a few other chrono formatters.

libcxx/include/__format/container_adaptor.h
40 ↗(On Diff #506080)

These containers can contain floating-point values too.

libcxx/include/__format/format_functions.h
442

Interesting. I guess that explains why I had to use the availability macro at a lot of places.

Can we add a TODO FMT Make non-templated when removing _LIBCPP_AVAILABILITY_FORMAT.

libcxx/include/__format/format_parse_context.h
62–63

Is the removal of _LIBCPP_AVAILABILITY_FORMAT really correct since the format_error's destructor in experimental library. I haven't looked whether basic_format_arg can throw. I know for certain formatter::parse can throw.

libcxx/include/__format/formatter_tuple.h
42 ↗(On Diff #506080)

A tuple may contain a floating-point value.

libcxx/include/__format/range_default_formatter.h
87 ↗(On Diff #506080)

The sequence, map, and set version may be used with floating-point types.

171 ↗(On Diff #506080)

If it doesn't hurt I prefer to keep this one and remove it in D145853 instead.

libcxx/include/__format/range_formatter.h
43 ↗(On Diff #506080)

_Tp may be a floating-point type.

libcxx/test/libcxx/utilities/format/format.availability.verify.cpp
15 ↗(On Diff #506080)

Just curious, do we have these tests for other availability macros too?

libcxx/test/std/time/time.syn/formatter.month.pass.cpp
14

Is this on your list to investigate?

ldionne marked 8 inline comments as done.Mar 17 2023, 3:07 PM
ldionne added inline comments.
libcxx/include/__availability
266 ↗(On Diff #506080)

I am not certain I understand the question. I went through your comments but I don't remember seeing something related to removal of the macro in classes. Can you clarify?

libcxx/include/__chrono/ostream.h
95

If it does, then that would be guarded by the availability macros on std::to_chars(floating-point) itself. IOW, if this ends up calling something that could be missing from the dylib on your deployment target, it'll be caught because std::to_chars(floating-point) is annotated properly.

libcxx/include/__format/container_adaptor.h
40 ↗(On Diff #506080)

As we discussed: yes, but in that case, the code will fail to compile due to the availability markup on the floating point formatter instantiated within. But using a container adaptor without any floating point in it should work, and it shouldn't fail to compile.

libcxx/include/__format/format_functions.h
442

We can add a TODO, however it would likely be super long lived since we probably won't remove _LIBCPP_AVAILABILITY_FORMAT before a long long time, aka when we drop support for the OS in which those will first be introduced. That'll be years.

However, I think what we should do is fix this problem in Clang, since that's clearly (?) a Clang bug.

libcxx/include/__format/format_parse_context.h
62–63

Since that's a static library, the destructor is effectively compiled into the user's binary, removing the deployment target requirement. Putting it in a static library is as-if it has been defined in the headers from a where-the-code-ends-up-compiled perspective.

libcxx/include/__format/formatter_tuple.h
42 ↗(On Diff #506080)

Same answer as for container adaptors!

libcxx/include/__format/range_default_formatter.h
87 ↗(On Diff #506080)

Same answer as for container adaptors!

171 ↗(On Diff #506080)

I guess I am fine with either, but I don't understand why you'd remove that in D145853. If your concern is about merge conflicts, I am fine with rebasing this change on top of yours. Leaving comment open to determine what to do.

libcxx/include/__format/range_formatter.h
43 ↗(On Diff #506080)

Same answer as for container adaptors!

libcxx/test/libcxx/utilities/format/format.availability.verify.cpp
15 ↗(On Diff #506080)

We have a few, for example libcxx/test/libcxx/thread/barrier.availability.verify.cpp.

libcxx/test/std/time/time.syn/formatter.month.pass.cpp
14

Yes, I would like to get to it eventually, but I won't lie to you, it'll take me some time.

ldionne updated this revision to Diff 506206.Mar 17 2023, 3:11 PM
ldionne marked 8 inline comments as done.

Fix CI issue and rebase.

ldionne updated this revision to Diff 506287.Mar 18 2023, 5:08 AM

Fix a few filesystem tests failing on old deployment targets

ldionne updated this revision to Diff 506392.Mar 19 2023, 7:26 AM

Rebase onto main.

This really starts to look nice, there are a few minor issues. After that I want to have look at the final patch before approving.

libcxx/include/__availability
266 ↗(On Diff #506080)

Mainly why we can now remove all AVAILABILITY macros in the classes. I think it's mainly a conclusion for me since you answered the other questions.

libcxx/include/__format/container_adaptor.h
40 ↗(On Diff #506080)

I think the only thing that puzzles my then is why not remove the _LIBCPP_AVAILABILITY_FORMAT from libcxx/include/__format/formatter_floating_point.h and and let that use transitive depend on to_char(floating-point).

libcxx/include/__format/format_functions.h
442

That makes me want a TODO FMT even more and then a link to the Clang bug-report or the rdar number.

libcxx/test/libcxx/utilities/format/format.availability.verify.cpp
15 ↗(On Diff #506080)

Thanks, I hadn't noticed these before.

libcxx/test/std/time/time.syn/formatter.month.pass.cpp
14

It would be interesting to know what you found thusfar, maybe discuss it on our next 1:1. I don't see this part as a blocker.

I would like to see TODO FMT instead of just TODO. This is consistent with the other format related TODOs.

ldionne marked 5 inline comments as done.Mar 20 2023, 2:55 PM
ldionne added inline comments.
libcxx/include/__availability
266 ↗(On Diff #506080)

I don't think any of the classes were using std::to_chars(floating-point) directly, so that's why I removed the annotations from them.

libcxx/include/__format/container_adaptor.h
40 ↗(On Diff #506080)

That's actually a great point. I think I can go one step further and remove _LIBCPP_AVAILABILITY_FORMAT altogether. If you use something from <format> and it ends up using std::to_chars(floating-point), then so be it, you'll get a compiler error. If not then no compiler error. I think that actually makes the most sense.

If we have a deployment requirement for std::format itself, then we can re-add markup.

libcxx/include/__format/format_functions.h
442

Ok, I reduced and created http://llvm.org/PR61563. I'll link to it.

ldionne updated this revision to Diff 506736.Mar 20 2023, 2:56 PM
ldionne marked 2 inline comments as done.
ldionne retitled this revision from [libc++] Fix availability markup for std::format to [libc++] Remove availability markup for std::format.
ldionne edited the summary of this revision. (Show Details)

Remove format-specific availability markup, which isn't actually needed.

Mordante added inline comments.Mar 21 2023, 10:40 AM
libcxx/include/__format/container_adaptor.h
40 ↗(On Diff #506080)

I agree, removing it altogether was on my mind too, but I wasn't sure whether it was possible, hence the question regarding the floating_point formatter. I assume you intend to remove them everywhere so I will not review this version further. Please let me know if you want me to review this version first.

I think I will move the format_error destructor to the header, then we won't need an availability macro. I'm not sure whether that will impact Apple users. But let's discuss that at another time.

libcxx/include/__format/format_functions.h
442

Thanks!

ldionne marked an inline comment as done.Mar 22 2023, 8:35 AM
ldionne added inline comments.
libcxx/include/__format/container_adaptor.h
40 ↗(On Diff #506080)

In the current state of this patch, all _LIBCPP_AVAILABILITY_FORMAT should be gone. I think I would like to get this one reviewed and landed and then I will rebase the other patches in the stack on top of it. Does that make sense?

ldionne updated this revision to Diff 507376.Mar 22 2023, 8:55 AM
ldionne marked an inline comment as done.

Rebase and fix formatting.

Mordante accepted this revision.Mar 22 2023, 9:23 AM

LGTM!

libcxx/include/__format/container_adaptor.h
40 ↗(On Diff #506080)

Yes that sounds good. I just wanted to avoid spending time reviewing, with the chance the patch would be reworked again. The current patch is indeed what I expected the patch would look like :-)

libcxx/include/__format/range_default_formatter.h
171 ↗(On Diff #506080)

It would prevent a merge conflict, but since you remove the macro from __availability I will have to deal with the conflict.

This revision is now accepted and ready to land.Mar 22 2023, 9:23 AM
ldionne updated this revision to Diff 507397.Mar 22 2023, 9:29 AM

Try to re-trigger CI. I don't understand why the job is skipped and the CI reports as green.

Looks like this passed, shipping.

This revision was landed with ongoing or failed builds.Mar 22 2023, 1:33 PM
This revision was automatically updated to reflect the committed changes.