-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add MAX_LEN_UTF8
and MAX_LEN_UTF16
Constants
#120580
base: master
Are you sure you want to change the base?
Add MAX_LEN_UTF8
and MAX_LEN_UTF16
Constants
#120580
Conversation
MAX_LEN_UTF8
and MAX_LEN_UTF16
constantsMAX_LEN_UTF8
and MAX_LEN_UTF16
Constants
r? @m-ou-se (rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #121295) made this pull request unmergeable. Please resolve the merge conflicts. |
In the tracking issue, please include a mention that when stabilizing this we should adjust the documentation of https://doc.rust-lang.org/std/primitive.char.html#method.encode_utf8 to use it. |
We briefly discussed this in the libs-api meeting and are happy to add these as unstable. |
Tracking issue created. Will finish the remaining things later today. |
177bcc3
to
ae30dc9
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
f51dde5
to
bee9b68
Compare
@rustbot ready |
☔ The latest upstream changes (presumably #116113) made this pull request unmergeable. Please resolve the merge conflicts. |
Small question: why are these constants in the |
From what I can tell, in this PR they are added as associated constants on |
Oh, I completely missed that point. I personally wouldn't add module exports since that way there's just one way to access them. We've also been deprecating the module equivalents of associated consts in a bunch of places, and so adding a new one doesn't make much sense. |
bee9b68
to
4b750dc
Compare
4b750dc
to
c803be1
Compare
☔ The latest upstream changes (presumably #130165) made this pull request unmergeable. Please resolve the merge conflicts. |
Observation: we still don't support importing associated constants in order to use them unqualified, and these seem like the kinds of things people might want to import and use unqualified. Until we support importing associated constants, I think we should add these as both. We can always deprecate the module constants in the future when we do add that support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We briefly discussed this in the libs-api meeting. This can be merged after rebasing.
Recording an observation from a discussion in a meeting: I found This isn't a blocking objection, on the basis that anytime I'm using UTF-16 I'm already having a bad time, so this doesn't make it that much worse. I just hope that nobody ever uses this to size e.g. a buffer for a |
This pull request adds the
MAX_LEN_UTF8
andMAX_LEN_UTF16
constants as per #45795, gated behind thechar_max_len
feature.The constants are currently applied in the
alloc
,core
andstd
libraries.