-
Notifications
You must be signed in to change notification settings - Fork 14k
Add String<A> type with custom allocator parameter
#149328
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
base: main
Are you sure you want to change the base?
Conversation
|
The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes. cc @BoxyUwU, @jieyouxu, @Kobzol, @tshepang This PR modifies Some changes occurred in src/tools/clippy cc @rust-lang/clippy rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer |
|
rustbot has assigned @Mark-Simulacrum. Use |
|
maybe r? @Amanieu as the reviewer of the previous PR has more context edit: whoops saw previous reviewer actually was Mark-Simulcrum, Amanieu was just last to leave a review. If it makes sense to swap it back, please do! |
This comment has been minimized.
This comment has been minimized.
This is a third attempt at implementing adding Allocator support to the std lib's `String`. Still stuck on the same issue with type inference failing on the newly generic `String<A>`, but I opted to do even less than the previous WIP work, and have added no new functions (`String<A>` can be constructed via `Vec<u8, A>` or `Box<str, A>`), and have moved only the struct definition to its own mod to make rebasing a bit easier if/when main changes underneath me.
This adds a diagnostic name `string_in_global` so that clippy can easily check for the alloc::string::String type alias.
This comment has been minimized.
This comment has been minimized.
added some code in linkchecker to check the generic::String docs when trying to resolve links to alloc::string::String type alias. There's some lazy-loading that the browser does, but linkchecker doesn't, so maybe some general-purpose solution would be better, but this seemed better than a big list of exceptions.
Yes, you could just use `unsafe { from_utf8_unchecked }``, but people get
antsy about `unsafe`, so add some Vec ctor/dtor equivalents.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☔ The latest upstream changes (presumably #147799) made this pull request unmergeable. Please resolve the merge conflicts. |
This change is part of the
allocator_apifeature set #32838 (also see wg-allocators roadmap or libs-team ACP).The previous attempts at adding an allocator parameter to string are at rust-lang/rust#101551, and rust-lang/rust#79500 (I think those authors should get much of the credit here, I am re-writing what they worked out in those threads).
workaround
There is a type inference ambiguity introduced when adding a generic parameter to a type which previously had none, even when that parameter has a default value (more details in rust-lang/rust#98931). I've done the same workaround as rust-lang/rust#101551, which is to make
alloc::string::Stringa type alias toString<Global>, but I've arranged the modules a bit differently to make rebase/merges a bit easier.This workaround unfortunately changes the type name of the
Stringlanguage item, and that would be user-facing in error or diagnostic output. I understand from this comment that this change is acceptable.changes to existing API
Most of the methods on the original
Stringhave been implemented for the generic version instead. I don't foresee anything more moving fromString<Global>toString<A>, as the remaining methods are all constructors which implicitly use theGlobalallocator.There are three general types of changes:
impl Foo for Stringtoimpl<A: Allocator> Foo for String<A>Vec<u8, A>andBox<str, A>: here we can use the existing allocator from those types.String<A>,String<Global>, orString<impl Allocator + Default>, etc; in general I try to leave these out of this change, but where some analogous change was made toVec<T, A>I follow that.new methods
Some methods have been added to
String<A>which are not strictly necessary to land this change, but are considered helpful enough to users, and close enough to existing precedent inVec<T, A>. Specifically, 4 new constructors (new_in,with_capacity_in,try_with_capacity_in,from_raw_parts_in), 1 destructor (into_raw_parts_with_alloc), and 1 getter (allocator). Technically, the updatedfrom_utf8_uncheckedshould be sufficient for constructing, but we can add some safe constructors so users don't have to sully themselves.not implemented
Variants of
from_utf{8,16}*which internally allocate or useCowhave been punted on this PR, maybe a followup would make sense to either rewrite them, or add some*_invariant.