[WIP] Add support for custom allocator for String
#101551
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
the
T-libs
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
zachs18
force-pushed
the
string_alloc
branch
from
6faca17
to
a2b102a
Compare
zachs18
commented
Sep 8, 2022
Comment on lines
7
to
11
| | | expected struct `String`, found `&String` | ||
| | arguments to this function are incorrect | ||
| | | ||
| = note: expected struct `String` | ||
| found reference `&String` |
There was a problem hiding this comment.
This PR changed a lot of the UI tests/diagnostics to have duplicated-ish note messages. I don't know enough about the diagnostics system to really understand why. Are these changes acceptable?
(Several similar changes, e.g. expected struct String, found type str in src/test/ui/issues/issue-53348.stderr, expected struct String, found unit type () in src/test/ui/block-result/consider-removing-last-semi.stderr)
There was a problem hiding this comment.
I also don't know the cause here, but it feels like it might be a larger bug -- maybe worth trying to reproduce on stable/nightly outside of the String type (e.g., have a struct and add a generic parameter to it) and see if that causes similar behavior on regular rustc compilers? If so, then you could file an issue for that.
If this is something specific to String that's probably worth poking at too.
There was a problem hiding this comment.
It does not appear to be specific to String; I have opened #101992 about it.
This comment has been minimized.
This comment has been minimized.
zachs18
force-pushed
the
string_alloc
branch
from
a2b102a
to
7ab2548
Compare
This comment has been minimized.
This comment has been minimized.
Things that are the same as 79500:The following were made allocator-aware (or added) in the same way as in 79500:
DIfferences from 79500 :
Additions from 79500:
|
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
|
It passes the checks now, so I'm marking this as ready for review. Feedback is appreciated! (especially on the unresolved questions in the Todos). (CC'ing people from 79500) |
|
(I think this is right?) |
rustbot
added
T-libs-api
library/alloc/src/string.rs
Outdated
| #[inline] | ||
| #[unstable(feature = "allocator_api", issue = "32838")] | ||
| #[must_use] | ||
| pub fn with_capacity_in(capacity: usize, alloc: A) -> String<A> { |
There was a problem hiding this comment.
I sort of feel like we shouldn't provide these APIs and instead encourage String::from(Vec::with_capacity_in(....)), since they do add noise to documentation and most users aren't using them. But that can be handled at stabilization time, I suppose, and is a broader question about everything being allocator-generic.
library/alloc/src/string.rs
Outdated
|
|
||
| #[cfg(not(no_global_oom_handling))] | ||
| #[stable(feature = "from_char_for_string", since = "1.46.0")] | ||
| impl From<char> for String { | ||
| // FIXME(zachs18): A: Allocator + Default? |
There was a problem hiding this comment.
I don't think we should add this comment; there's many APIs which could do this (even on Vec) and it doesn't seem worth leaving comments strewn around std for that future possibility.
There was a problem hiding this comment.
Yeah, I had intended to remove or implement most of the FIXMEs based on feedback before the PR gets merged. I'll just remove the ones that don't currently exist for Vec for this PR.
|
A few notes:
I think this is not observable on stable, though I'm not entirely confident. The primary area where it matters that I know of is pattern matching with a const value, but that requires StructuralPartialEq too.
In order to check whether this is a breaking change we need to know whether any const context on stable can get the value -- I think the answer may be no, but I'm not confident. Also left some comments inline, going to @rustbot author for now but please mark it as ready for review if the ACP goes through and you have addressed comments (or posted back with questions). |
rustbot
added
S-waiting-on-author
zachs18
force-pushed
the
string_alloc
branch
from
0445581
to
311f4a5
Compare
Amanieu
reviewed
Sep 20, 2022
This comment has been minimized.
This comment has been minimized.
zachs18
force-pushed
the
string_alloc
branch
from
311f4a5
to
c75bd1f
Compare
This comment has been minimized.
This comment has been minimized.
|
Still more work to do to to get clippy/tests/etc working with having |
|
@zachs18 any updates on this? thanks |
More `allocator_api`-aware `String` implementations (Most allocator-irrelevant associated functions, Drain, and FromUtf8Error). More `allocator_api`-aware `String` implementations, including added `new_in` etc inherent impls. tidy, new_in etc, raw_parts_in etc, allocator(&self), FromUtf8Error impls, Extend, Pattern, PartialEq &str etc with String<A>, Display, Debug, Hash, Add(Assign), ToString, AsRef/AsMut, conversions Box<str, A> -> String<A>, &String -> Cow<str>, String<A> -> Vec<u8, A>; fmt::Write. Fix gdb/lldb integration for String<A>. Add some simple trait impls I missed. Borrow(Mut)<str> for String, From<String<A>> for Rc<str>/Arc<str>, Clone for Box<str, A>. Remove FIXMEs for String<A> APIs that don't already exist for Vec<T,A>. Rewrite `Clone for Box<str, A>` to avoid extra capacity/length checks converting from Vec. Also implement `clone_from` to re-use the allocation when the lengths are the same. Remove `From<String<A>> for Rc<str>`/`Arc` impls to allow for future alloc-ification of `Rc`/`Arc` Note: find out how to make "the full type name has been written to" errors be deterministic (./x.py test alone didn't do it). AsRef<OsStr> and AsRef<Path> for String<A: Allocator>
…alias with the generic filled in.
TODO: try to modify the compiler to remove the <A> from "help: the trait `Clone` is implemented for `String<A>`".
…eneric parameter to String.
… to fix them right now, so just adding the changes so I can look at other errors. Fix these before merging.
This comment has been minimized.
This comment has been minimized.
…str, A> to A = Global.
This comment has been minimized.
This comment has been minimized.
…o the new path. Also reverts some UI test changes that were due to on_unimplemented path being wrong.
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☔ The latest upstream changes (presumably #125443) made this pull request unmergeable. Please resolve the merge conflicts. |
Dylan-DPC
added
S-blocked
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Roadmap for allocator support.
API Change Proposal: rust-lang/libs-team#101 [ Accepted ]
A subset/bring-to-current of #79500 utilizing a workaround(?) to the issue (#79499) that was blocking it.
(Note: I previously tried rebasing that (2 year old) PR onto current master, which was not my best idea).
Blocked on:
Stringlibs-team#101Todo/Unresolved Questions:
Fix UI tests.Done?(see my question below)(see Apparent duplicated diagnostic note for type mismatch involving type with defaulted type parameter. #101992)String<A>.ShouldremovedFrom<String<A>> for Rc<str>/Arcbe removed from this PR? (Corresponding impl does not exist forVec/Rc<[T]>, and also it may preclude a futureFrom<String<A>> for Rc<str, A>)impl StructuralEq for String<_>be re-added? This PR removes it (because it no longerderive(Eq)), but it may not be observable becauseString: !StructuralPartialEqFromUtf8Error<_>, but that derived bothPartialEqandEqso it had bothStructuralPartialEqandStructuralEq.CString, which derivesPartialEqandEq.)impl<A: Allocator> From<String<A>> to Box<str, A>breaks orphan rules? (becauseBoxandstrare fundamental?)String<A>?fn String<A>::from_str_in(s: &str, alloc: A) -> String<A>?fn str::to_string_in(alloc: A) -> String<A>(parallelsslice::to_vec_in)to_string_intoToString? (Probably a non-starter, since that would make ToString not object-safe, unless we put awhere Self: Sized, which would precludestr)[ ] Other(removed FIXME comments, APIs moved to future work)FIXMEs (implement or remove comments)Possible Future work
from_utf8_lossy, which returns a non-allocator-awareCow<str>?from_utf16(_lossy), which returnStringandResult<String, _>?String<A> where A: Defaultfrom_utf16(_lossy)_in, (Add support for custom allocator for(C)String#79500 only addedfrom_utf16_in, not the lossy version)From<Cow<str>> for String<A> where A: Defaultcould be implemented asfrom_str_in(&*cow, A::default()), and specialized forA = Globalto the current implementationcow.into_owned().From<&String<A>> for String<A> where A: CloneorFrom<&String<A>> for String<B> where B: Default(I don't think both can exist).FromIterator<_> for String,FromStr for String, andFrom<&str-like> for Stringcould befor String<A> where A: Default.impl const Default for Stringcould beimpl<A: _ + ~const Default> Default for String<A>, except thatGlobaldoesn't implementconst Defaultcurrently (but I don't see any reason it couldn't).