Skip to content

Implement ByteStr and ByteString types #135073

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

Merged
merged 8 commits into from
Jan 24, 2025
Merged

Conversation

joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Jan 3, 2025

Approved ACP: rust-lang/libs-team#502
Tracking issue: #134915

These types represent human-readable strings that are conventionally,
but not always, UTF-8. The Debug impl prints non-UTF-8 bytes using
escape sequences, and the Display impl uses the Unicode replacement
character.

This is a minimal implementation of these types and associated trait
impls. It does not add any helper methods to other types such as [u8]
or Vec<u8>.

I've omitted a few implementations of AsRef, AsMut, Borrow,
From, and PartialOrd, when those would be the second implementation
for a type (counting the T impl) or otherwise may cause inference
failures. These impls are important, but we can attempt to add them
later in standalone commits, and run them through crater.

In addition to the bstr feature, I've added a bstr_internals feature
for APIs provided by core for use by alloc but not currently
intended for stabilization.

This API and its implementation are based heavily on the bstr crate
by Andrew Gallant (@BurntSushi).

r? @BurntSushi

@joshtriplett joshtriplett added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jan 3, 2025
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 3, 2025
@rust-log-analyzer

This comment has been minimized.

@joshtriplett

This comment was marked as resolved.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@joshtriplett

This comment was marked as resolved.

@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum

This comment was marked as resolved.

@joshtriplett joshtriplett added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. needs-crater This change needs a crater run to check for possible breakage in the ecosystem. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 5, 2025
@joshtriplett joshtriplett force-pushed the bstr branch 2 times, most recently from b539a8c to f035281 Compare January 5, 2025 16:51
@rust-log-analyzer

This comment has been minimized.

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jan 13, 2025
@joshtriplett
Copy link
Member Author

joshtriplett commented Jan 13, 2025

Well, that's definitive.

This is ready for review.

@joshtriplett joshtriplett removed the needs-crater This change needs a crater run to check for possible breakage in the ecosystem. label Jan 13, 2025
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!

Was there discussion on ByteStr verus BStr? I guess the latter is a bit more opaque, so I think ByteStr is probably the right choice for std. I do like the succinctness of BStr though. (And similarly for ByteString versus BString.)

@Sky9x
Copy link
Contributor

Sky9x commented Jan 22, 2025

Could an impl CloneToUninit for ByteStr be added? cc #126799

@joshtriplett
Copy link
Member Author

This is awesome!

Thank you!

Was there discussion on ByteStr verus BStr? I guess the latter is a bit more opaque, so I think ByteStr is probably the right choice for std. I do like the succinctness of BStr though. (And similarly for ByteString versus BString.)

I like the succinctness of BStr and BString as well, but others in the libs-api meeting felt those were too opaque. The deciding factor that led me to not push further for the shorter names was that the longer names don't conflict with the bstr crate, making it safer to (for instance) place them in the prelude in the future, and making it easier for crates that need to provide impls for both.

@joshtriplett
Copy link
Member Author

Could an impl CloneToUninit for ByteStr be added? cc #126799

Sure, I can do that.

@Skgland
Copy link
Contributor

Skgland commented Jan 22, 2025

I noticed that ByteString isn't allocator aware. Is this intentional? Going from non to one (defaulted) generic argument is making the addition of allocator awareness difficult for String in #101551 (though that would probably only be a problem if ByteString didn't become allocator aware prior to stabilization which is still far out with it not even being merged to nightly).

@joshtriplett
Copy link
Member Author

joshtriplett commented Jan 22, 2025

@Skgland No objection to adding that, but that would be a complex change on top of this, so I would prefer to defer it to another PR.

@BurntSushi
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 23, 2025

📌 Commit 865471f has been approved by BurntSushi

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 23, 2025
@bors bors merged commit 08d5b23 into rust-lang:master Jan 24, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 24, 2025
@joshtriplett joshtriplett deleted the bstr branch January 24, 2025 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

10 participants