Skip to content
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

core::net: Rename parse_ascii() fns to from_bytes() #117274

Closed
wants to merge 1 commit into from

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Oct 27, 2023

@rustbot
Copy link
Collaborator

rustbot commented Oct 27, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 27, 2023
We are operating on u8 (aka. byte) values here. ASCII only allows values from 0-127.

This would also be consistent with the naming of `OsStr::from_bytes()`.
@Turbo87 Turbo87 force-pushed the rename-parse-ascii branch from 470714f to 7783695 Compare October 27, 2023 10:23
@cuviper
Copy link
Member

cuviper commented Oct 27, 2023

We are operating on u8 (aka. byte) values here. ASCII only allows values from 0-127.

Are any non-ASCII bytes actually accepted Ok though?

This would also be consistent with the naming of OsStr::from_bytes().

That's an infallible conversion, truly accepting any bytes.

Anyway, I think this changes posture enough that it needs to be an API decision, especially given that there's a proposed generalization in play at rust-lang/libs-team#287.

@rustbot label -T-libs +T-libs-api
r? libs-api

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 27, 2023
@rustbot rustbot assigned joshtriplett and unassigned cuviper Oct 27, 2023
@Turbo87
Copy link
Member Author

Turbo87 commented Oct 27, 2023

Are any non-ASCII bytes actually accepted Ok though?

fair point, I guess... 🤔

@jmillikin
Copy link
Contributor

jmillikin commented Oct 28, 2023

It seems confusing for IpAddr::from_bytes(&[58, 58, 48, 49]) and IpAddr::from([58, 58, 48, 49]) to have different behavior .

@scottmcm
Copy link
Member

When I see "from bytes" I think https://doc.rust-lang.org/std/primitive.u32.html#method.from_ne_bytes, not anything related to parsing. So I think it needs more in the name to clarify the intent of the bytes.

@Turbo87 Turbo87 closed this Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants