Page MenuHomePhabricator

[libc++] Implement P1423R3 (char8_t backward compatibility remediation)
ClosedPublic

Authored by philnik on Jul 6 2022, 5:53 AM.

Diff Detail

Event Timeline

philnik created this revision.Jul 6 2022, 5:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2022, 5:53 AM
Herald added a subscriber: arichardson. · View Herald Transcript
philnik requested review of this revision.Jul 6 2022, 5:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2022, 5:53 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I like to see what happens in the CI. It's a bit curious no existing tests seem to be broken.

libcxx/docs/Status/Cxx20Papers.csv
118

Make sure to set the proper version number before landing.

libcxx/include/ostream
1134

Please update the synopsis.

1136

Next time for reviewing it would be easier to use the same order as the paper.

1168

This char8_t needs to be guarded.

1176

These wchar_t need to be guarded.

libcxx/test/std/input.output/iostream.format/output.streams/ostream/deleted_output_functions.verify.cpp
9

This should be restricted to version 20 and newer.

22–23

For clarity I would prefer and similar changes to other tests.

Though you probably know, there's yet another DR on top of P1423 that has been accepted (only missing plenary vote): https://github.com/cplusplus/papers/issues/1171

Might be worth investigating to not needlessly introduce breakage that will then have to be undone again.

philnik updated this revision to Diff 444654.Thu, Jul 14, 7:34 AM
philnik marked 5 inline comments as done.
  • Address comments
libcxx/include/ostream
1136

I first put it in the same order as in the paper, but that would have meant 3x the amount preprocessor directives, which would be a lot less readable in the long term IMO.

libcxx/test/std/input.output/iostream.format/output.streams/ostream/deleted_output_functions.verify.cpp
22–23

I think explicitly naming the types makes it a lot clearer. I have to think a lot harder to know what u8, u, U and L mean (I actually had to look these up).

Though you probably know, there's yet another DR on top of P1423 that has been accepted (only missing plenary vote): https://github.com/cplusplus/papers/issues/1171

Might be worth investigating to not needlessly introduce breakage that will then have to be undone again.

The paper you linked only changes core wording, so it shouldn't make any difference for the library part. Or am I missing something?

Though you probably know, there's yet another DR on top of P1423 that has been accepted (only missing plenary vote): https://github.com/cplusplus/papers/issues/1171

Might be worth investigating to not needlessly introduce breakage that will then have to be undone again.

The paper you linked only changes core wording, so it shouldn't make any difference for the library part. Or am I missing something?

It's more likely me who missed something, but I just thought I'd point out that paper since it builds on top of P1423 and also bumps that feature macro AFAICT.

SGTM, but I like to have a quick look after the CI passes. Feel free to ping me on Discord when the CI is green.

libcxx/include/ostream
1136

That might be true, I can only say that when I would see that version. But at least the synopsis was a breeze to review :-)

libcxx/test/std/input.output/iostream.format/output.streams/ostream/deleted_output_functions.verify.cpp
22–23

For me it's exactly the other way around, for these types I have to look at a lot of "noise" to see the type. But I don't feel this is wrong so feel free to keep it as-is unless other reviews also prefer L and friends.

22–23

I expect that is cause of the CI failures, but I guess you need to use more guards since sw is used unguarded below.

25

Mainly curious, but will std::declval<wchar_t*>(); be valid? I expect it will.

philnik updated this revision to Diff 445342.Sun, Jul 17, 11:52 AM
philnik marked 5 inline comments as done.
  • Address comments
libcxx/test/std/input.output/iostream.format/output.streams/ostream/deleted_output_functions.verify.cpp
25

No it isn't valid. The deleted const wchar_t* overload is the best match.

Mordante accepted this revision.Mon, Jul 18, 9:16 AM

LGTM with one question. Please make sure to update the version number before landing.

libcxx/test/std/input.output/iostream.format/output.streams/ostream/deleted_output_functions.verify.cpp
25

How do you feel about adding explicit test cases?

This revision is now accepted and ready to land.Mon, Jul 18, 9:16 AM
philnik updated this revision to Diff 445659.Mon, Jul 18, 5:03 PM
philnik marked 2 inline comments as done.
  • Address comments
libcxx/test/std/input.output/iostream.format/output.streams/ostream/deleted_output_functions.verify.cpp
25

Sure, I don't expect CharT* and const CharT* to ever have different behaviour. I think that would be very surprising.

Mordante added inline comments.Tue, Jul 19, 1:15 PM
libcxx/test/std/input.output/iostream.format/output.streams/ostream/deleted_output_functions.verify.cpp
25

The Standard has some hidden gems where the const matters ;-)
https://cplusplus.github.io/LWG/issue3701

But generally I agree.