Details
- Reviewers
ldionne Mordante var-const - Group Reviewers
Restricted Project - Commits
- rG309aed306817: [libc++] Implement P1423R3 (char8_t backward compatibility remediation)
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
- 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). |
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. |
- 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. |
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? |
- 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. |
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 ;-) But generally I agree. |
Make sure to set the proper version number before landing.