Add String<A> type with custom allocator parameter#149328
Add String<A> type with custom allocator parameter#149328shua wants to merge 8 commits intorust-lang:mainfrom
String<A> type with custom allocator parameter#149328Conversation
|
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
added
A-rustc-dev-guide
|
rustbot has assigned @Mark-Simulacrum. Use |
rustbot
added
S-waiting-on-review
|
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 comment has been minimized.
This comment has been minimized.
rustbot
added
the
T-release
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #147799) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
|
☔ The latest upstream changes (presumably #149836) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #150546) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes made this pull request unmergeable. Please resolve the merge conflicts. |
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
I can probably rebless the test output this week. Afaict, none of these merge conflicts are related to this change, just a fallout of the type name changing so stdout/err changes for any tests that use It's a pain to rebless every conflict, |
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 PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
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 links are changed in the original source, so not sure why the html being checked in CI still has them, maybe it checks docs from `main` as well, but if so, wouldn't `struct.String.html` still exist? Truly a pickle, but I'll add these exceptions and a note.
|
a note for my future self: these tests fail locally, but do not fail in CI
|
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #151501) made this pull request unmergeable. Please resolve the merge conflicts. |
8 participants
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.