allocator: refactor for stabilisation - #157428
Conversation
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@rustbot author (mostly so you can more clearly signal when you think things are ready; I've commented here already so I'll see any additional changes for review as they're made) |
This comment has been minimized.
This comment has been minimized.
| @@ -74,7 +74,8 @@ impl fmt::Display for AllocError { | |||
| /// * Moving, subtyping, unsize-coercing, or trait-upcasting an allocator does not change | |||
| /// what the allocator is equivalent to. | |||
| /// * Copying or cloning allocator results in an allocator that's | |||
There was a problem hiding this comment.
Do we require copies to be equivalent? If so, this relies on the orphan rule for Copy (which might be fine since Copy is special), but I'm confused because the AllocatorClone docs do not talk about requirements for copies.
There was a problem hiding this comment.
I thought there was wording to the effect of "if Copy could be implemented for an AllocatorClone type, that must also obey its semantics", but if not I will add it. ty!
There was a problem hiding this comment.
What do you mean by "could"? Can we rely on Drop + Copy being disallowed for soundness? I think we didn't discuss this part enough 😅
This comment has been minimized.
This comment has been minimized.
yk what we can do btree later raaaaagh pain and suffering *unleaks your box* whats thiSegmentation fault (core dumped) straight up testing it. and by it, well, let's just say. "my allocator" linked and listed the unnecesary bits? gone. reduced to atoms
a41958f to
acb148f
Compare
|
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. |
|
(apologies for the squash-and-rebase, nothing meaningful to review should have changed between force-pushes) |
This comment has been minimized.
This comment has been minimized.
| if self.len() > source.len() { | ||
| self.split_off(source.len()); | ||
| } | ||
| for (elem, source_elem) in self.iter_mut().zip(&mut source_iter) { | ||
| for elem in self.iter_mut() { | ||
| let Some(source_elem) = source_iter.next() else { | ||
| break; | ||
| }; | ||
| elem.clone_from(source_elem); | ||
| } | ||
| while self.len() > source.len() { | ||
| self.pop_back(); | ||
| } |
There was a problem hiding this comment.
Unreasonably upset at this code just because of how limited the LinkedList API is. Should be doable, but isn't… :(
| /// This function is mainly useful for data that lives for the remainder of the program's life, | ||
| /// i.e., memory that is meant to leak. Reconstructing ("unleaking") a `Box` from the mutable | ||
| /// reference returned here (e.g. via [`Box::from_raw`]) is a grey area (meaning it is possible | ||
| /// under specific circumstances but many seemingly harmless ways of doing it are undefined | ||
| /// behavior) and should be avoided. If the memory should eventually be freed, prefer to use | ||
| /// [`Box::into_raw`] or [`Box::into_non_null`] instead. | ||
| /// | ||
| /// However, "unleaking" as per the above via [`Box::from_raw_in`] is only sound | ||
| /// for the global allocator. | ||
| /// |
There was a problem hiding this comment.
These two paragraphs combined read quite weird. I think you probably want to just separate the idea of "unleaking" into a second paragraph, starting with the no-unleak case and then clarifying unleaking only works for Global.
There was a problem hiding this comment.
It seems like this is an attempt to partially spell out what "specific circumstances" make unleaking sound. If that's what we want to do we should actually spell it out IMO:
Unleaking is allowed if
AisGlobal, and- the pointer passed to
from_rawwas derived from the reference returned by this method in a way that all intermediate references on the derivation path are&mut T(no shared references, no references to other types).
There was a problem hiding this comment.
that's better wording, yes. i'll adapt that, ty <3
There was a problem hiding this comment.
Oh and it may be worth reminding the reader that if a function takes an &mut argument, freeing that while the function runs is UB. So it'd be okay to to Box::from_raw inside such a function but not to drop that box.
There was a problem hiding this comment.
couldn't find a nice way to express this concisely so i'll just mention it's only sound for Global. that should at least be no more vague than it was before ^^
There was a problem hiding this comment.
That's fine, but why is it a separate paragraph? IMO the new sentence should go just after the "should be avoided".
| #[cfg(not(no_global_oom_handling))] | ||
| #[stable(feature = "box_slice_clone", since = "1.3.0")] | ||
| impl Clone for Box<str> { | ||
| impl<A: Allocator + Clone> Clone for Box<str, A> { |
There was a problem hiding this comment.
Would just comment here: not AllocatorClone since the new allocation doesn't have to be related to the original.
There was a problem hiding this comment.
happy to add that, sure thing
There was a problem hiding this comment.
Mostly mention because, my initial gut reaction was "wait, why not AllocatorClone?" and even though I figured it out, I figure just adding the extra comment helps.
There was a problem hiding this comment.
oh oops i thought i alr pushed the change i'd made here. yes hi i did it now and it's pushed
| #[unstable(feature = "allocator_api", issue = "32838")] | ||
| #[must_use] | ||
| #[inline] | ||
| pub unsafe fn from_boxed_utf8_unchecked_in<A: crate::alloc::Allocator>( |
There was a problem hiding this comment.
I still don't like this method, because I don't think we should use the _in naming when the allocator is part of a box, rather than as an extra parameter.
I assume there are problems with parameterising from_boxed_utf8_unchecked normally?
There was a problem hiding this comment.
I still don't like this method, because I don't think we should use the _in naming when the allocator is part of a box, rather than as an extra parameter.
Fair, happy to rename it and/or make it private, we just need the functionality for this clone impl.
I assume there are problems with parameterising from_boxed_utf8_unchecked normally?
Would be a breaking change to a stable api :c
There was a problem hiding this comment.
i guess i should explicitly ask - what would be preferred here? i'm fine with any option as long as I can get the semantics of this function
There was a problem hiding this comment.
I guess I would prefer just this being either private or a method on Box for now. I just don't want it public to imply this is the method we want; maybe with an edition redirect we could update the old method.
|
This PR seems to make multiple unrelated changes. Is it possible to get an overview of all the changes it's making? Or even better, perhaps we could split out this into multiple PRs so that we can merge the things we agree on now, without blocking them on the things still being discussed? |
|
I'm unsure how i'd go about splitting this up, since many of the miscellaneous library changes are necessary for the API to be sound. (ofc, if you have an idea that'd lighten the review burden, i'm happy to do it and i think @clarfonthey would be relieved too) In short, the changes made are:
i'm quite happy with the fact that (beyond adding the ban on unwinding out of an allocating method or drop) none of this tightens requirements on |
|
Also, unlike the old allocator api, we think this is sound (modulo some details in the docs we haven't discussed fully) :D |
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Ralf Jung <post@ralfj.de>
View all comments
Adds my current proposal per the doc in #156882 and follow-up Zulip conversations (notably for dyn-compat) unstably.
r? libs