This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Implement P2513
ClosedPublic

Authored by cor3ntin on Oct 21 2022, 6:44 AM.

Details

Summary

Implement P2513

This change allows initializing an array of unsigned char,
or char from u8 string literals.
This was done both to support legacy code and for compatibility
with C where char8_t will be typedef to unsigned char.

This is backported to C++20 as per WG21 guidance.

Diff Detail

Event Timeline

cor3ntin created this revision.Oct 21 2022, 6:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2022, 6:44 AM
cor3ntin requested review of this revision.Oct 21 2022, 6:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2022, 6:44 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
cor3ntin updated this revision to Diff 469595.Oct 21 2022, 7:00 AM

Remove unused diagnostic

Thanks for this! Can you add more details to the patch summary as to what this paper does? Paper numbers help, but don't convey a whole lot of information at a glance if we need to come back to this review for code archeology.

Please also add a test to the correct file in clang/test/CXX/drs/ and regenerate our C++ DR status page.

Are we missing changes for "with an integral conversion [conv.integral] if necessary for the source and destination value." ?

clang/lib/Frontend/InitPreprocessor.cpp
701

Why do we not want to go with 202207L regardless of language mode if this is being applied as a DR?

clang/lib/Sema/SemaInit.cpp
4466–4467

Unintentional indentation changes?

5858–5862

Unrelated change?

Thanks for this! Can you add more details to the patch summary as to what this paper does? Paper numbers help, but don't convey a whole lot of information at a glance if we need to come back to this review for code archeology.

Please also add a test to the correct file in clang/test/CXX/drs/ and regenerate our C++ DR status page.

I'll check but i don't think we have a number of that, we just decided to backport the paper

Are we missing changes for "with an integral conversion [conv.integral] if necessary for the source and destination value." ?

Nope, that's taken care of afaict, I'm trying to think of a test

Thanks for this! Can you add more details to the patch summary as to what this paper does? Paper numbers help, but don't convey a whole lot of information at a glance if we need to come back to this review for code archeology.

Please also add a test to the correct file in clang/test/CXX/drs/ and regenerate our C++ DR status page.

I'll check but i don't think we have a number of that, we just decided to backport the paper

Should we ask CWG for an issue # for it so that it's traceable whether we do/don't implement the DR, or is that overkill?

Are we missing changes for "with an integral conversion [conv.integral] if necessary for the source and destination value." ?

Nope, that's taken care of afaict, I'm trying to think of a test

Ah, a test would be appreciated then!

cor3ntin updated this revision to Diff 469690.Oct 21 2022, 11:00 AM

Improve commit message, check proper integral conversions,
remove ws changes.

cor3ntin edited the summary of this revision. (Show Details)Oct 21 2022, 11:01 AM
cor3ntin edited the summary of this revision. (Show Details)
cor3ntin edited the summary of this revision. (Show Details)
cor3ntin added inline comments.Oct 21 2022, 11:17 AM
clang/lib/Frontend/InitPreprocessor.cpp
701

There is a -fchar8_t that is an extension before C++20 and we are not touching that. It is pre-existing that the feature test macro was defined before c++20 if that flag was set.

aaron.ballman added inline comments.Oct 21 2022, 12:23 PM
clang/lib/Frontend/InitPreprocessor.cpp
701

Ahhhh, I see now that there's a (small) potential for breakage (thank you for that test case btw) and so you want to leave the extension flag behavior alone. We usually don't do that (usually these sort of extension flags enable the feature as it exists in the language mode enabling support for it) unless there's a good reason to do so. My thinking is:

The potential for breakage is small enough we're not even going to call it out as a potentially disruptive change in the release notes. Presuming there are likely more people using -std=c++20 than there are using -fchar8_t, changing -fchar8_t seems like less risky behavior than changing -std=c++20. So I think it's reasonable for us to enable this functionality for -fchar8_t as well because we're already assuming the larger risk.

WDYT?

cor3ntin updated this revision to Diff 469715.Oct 21 2022, 12:33 PM
cor3ntin edited the summary of this revision. (Show Details)

Enable the change in -fchar8_t mode as per
suggestion from Aaron.

aaron.ballman accepted this revision.Oct 24 2022, 5:44 AM

LGTM aside from a missing full stop in a comment. Tom's on vacation most of this week, so if he has any concerns, they can be address post-commit unless you want to wait for him (which is also totally fine by me).

clang/lib/Sema/SemaInit.cpp
94
This revision is now accepted and ready to land.Oct 24 2022, 5:44 AM
This revision was automatically updated to reflect the committed changes.

Thanks Aaron!