Add CartableOptionPointer and helpers to the yoke crate #4449
Conversation
There was a problem hiding this comment.
I'm opposed to this, for a bunch of reasons
Firstly, I think the use case here is rather specific; I don't think it's something that makes a lot of sense for most yoke users overall. If we get asks for something like this I'd like to design it based on the asks rather than genericizing the thing that ICU4X needs in a particular direction and assuming that that direction is what is needed.
Secondly, the abstraction itself is incredibly generic in a way that is both rather hard to understand for users (IMO) and rather hard to grapple with the safety of. I think we're introducing a lot of concepts here that interact in subtle ways.
(For this to land it is going to need a lot more careful documentation of the safety needs)
I don't agree that it is ICU4X-specific. This new cart type
Almost all of the unsafety is encapsulated in the new
Would like to pair with you today on writing the documentation. |
utils/yoke/src/cartable_ptr.rs
Outdated
| /// 1. The pointer MUST have been returned by this impl's `into_raw`. | ||
| /// 2. The pointer MUST NOT be dangling. | ||
| #[doc(hidden)] | ||
| unsafe fn clone_raw(pointer: NonNull<Self::Raw>); |
There was a problem hiding this comment.
nit (nb): I kinda prefer "addref" or something here
utils/yoke/src/cartable_ptr.rs
Outdated
| // called LICENSE at the top level of the ICU4X source tree | ||
| // (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ). | ||
|
|
||
| //! Types for optional pointers that may be dropped with niche optimization. |
There was a problem hiding this comment.
issue: this should be expanded a bit, at the very least pointing to the main type from this crate (CartableOptionPointer)
utils/yoke/src/cartable_ptr.rs
Outdated
|
|
||
| /// A type with similar semantics as `Option<C<T>>` but with a niche. | ||
| /// | ||
| /// Will panic on types with alignment 1. |
utils/yoke/src/cartable_ptr.rs
Outdated
| } | ||
| } | ||
|
|
||
| // Safety: logically an Option<C>. Has same bounds as Option<C> |
There was a problem hiding this comment.
nit: with my other PR landing you can potentially relate to the invariants specified there
Technically breaking, but I think this constness was a mistake and we don't use it internally (and there's very little reason for someone to use it externally, the static stuff exists). This is useful for #4449 ; it allows us to reference statics in the constructor of `DataPayload()` which lets us reference a *proper* sentinel rather than a made-up value that will not work in WASM.
Split from #4442
I do think this is in scope for
yokebecause the type can be a "one-way" type that is a more efficient cart than eitherOption<Rc<T>>orOption<ErasedRcCart>.