Conversation
axd1x8a
force-pushed
the
feat/impl-stl-ourselves
branch
2 times, most recently
from
e9979a6 to
f535d4a
Compare
axd1x8a
force-pushed
the
feat/impl-stl-ourselves
branch
from
8adce09 to
a1a9a1f
Compare
nex3
left a comment
nex3
left a comment
There was a problem hiding this comment.
-
I think it's probably worth adding unit tests for these, since they're very easy to isolate and somewhat algorithmically-intensive (particularly the RB tree).
-
I think all the collections should have
Dropimplementations even if we aren't constructing them ourselves yet, just so we don't shoot ourselves in the foot later by adding constructors. It's also possible that we'll end up owning some of them even without constructors (for example, if there's a collection of collections). -
I looked at all the red-black logic, but I can't say I verified it in great depth.
-
Personally I would find it clearer to put this directly within
cratesbut I don't feel that strongly about it.
Co-authored-by: Natalie Weizenbaum <nex342@gmail.com>
There was a problem hiding this comment.
Change this to publish fromsoftware-shared-stl instead
Co-authored-by: Natalie Weizenbaum <nex342@gmail.com>
axd1x8a
force-pushed
the
feat/impl-stl-ourselves
branch
3 times, most recently
from
cebdf96 to
44a19b1
Compare
crates/shared/stl/src/allocator.rs
Outdated
| /// | ||
| /// `size` must be non-zero. `align` must be a power of two. | ||
| /// The returned pointer is valid for `size` bytes and aligned to `align` | ||
| unsafe fn allocate_raw(&mut self, size: usize, align: usize) -> NonNull<c_void>; |
There was a problem hiding this comment.
Does this need to be unsafe? As long as the implementations do something sensible when the inputs are invalid (including erroring out) it should be safe.
Also, I don't think returning NonNull is correct here. It's possible for an allocator to return a null pointer if the allocation fails (and in fact we see checks for this all over the decomp).
There was a problem hiding this comment.
I still think it's weird to both have a distinction between allocate_raw and allocate (which suggests that allocate_raw is a much thinner abstraction over the native C API), while also having allocate_raw return NonNull when the C APIs are going to universally return null pointers to indicate allocation failures. I think it's better to make this
| unsafe fn allocate_raw(&mut self, size: usize, align: usize) -> NonNull<c_void>; | |
| unsafe fn allocate_raw(&mut self, size: usize, align: usize) -> *mut c_void; |
and handle the case where it returns null in allocate().
| use std::{cmp::Ordering, mem::MaybeUninit, ptr::NonNull}; | ||
|
|
||
| /// Comparator trait for use in MSVC `std::tree` [`RbTree`] | ||
| pub trait TreeComparator<V> { |
There was a problem hiding this comment.
I still think that writing ___.gte(___) is clearer than !____.lt(____) and so on...
Co-authored-by: Natalie Weizenbaum <nex342@gmail.com>
crates/shared/stl/src/allocator.rs
Outdated
| /// | ||
| /// `size` must be non-zero. `align` must be a power of two. | ||
| /// The returned pointer is valid for `size` bytes and aligned to `align` | ||
| unsafe fn allocate_raw(&mut self, size: usize, align: usize) -> NonNull<c_void>; |
There was a problem hiding this comment.
I still think it's weird to both have a distinction between allocate_raw and allocate (which suggests that allocate_raw is a much thinner abstraction over the native C API), while also having allocate_raw return NonNull when the C APIs are going to universally return null pointers to indicate allocation failures. I think it's better to make this
| unsafe fn allocate_raw(&mut self, size: usize, align: usize) -> NonNull<c_void>; | |
| unsafe fn allocate_raw(&mut self, size: usize, align: usize) -> *mut c_void; |
and handle the case where it returns null in allocate().
3 participants
No description provided.