Implement ZeroMapKV for Pattern<SinglePlaceholder, str>
#5030
Conversation
utils/pattern/src/implementations.rs
Outdated
|
|
||
| use crate::{Pattern, SinglePlaceholder}; | ||
|
|
||
| impl<'data> ZeroMapKV<'data> for Pattern<SinglePlaceholder, Cow<'data, str>> { |
There was a problem hiding this comment.
@sffc and @Manishearth
I would like if you can give me your help in this implementation.
There was a problem hiding this comment.
is it should be something like this ?
impl<'data> ZeroMapKV<'data> for Pattern<SinglePlaceholder, Cow<'data, str>> {
type Container = ZeroVec<Pattern<SinglePlaceholder, Cow<'data, str>>>;
type Slice = ZeroVec<Pattern<SinglePlaceholder, Cow<'data, str>>>;
type GetType = Cow<'data, str>;
type OwnedType = Cow<'data, str>;
}
There was a problem hiding this comment.
It might be more like
impl<'data> ZeroMapKV<'data> for Pattern<SinglePlaceholder, str> {
type Container = VarZeroVec<'data, Pattern<SinglePlaceholder, str>>;
type Slice = VarZeroSlice<Pattern<SinglePlaceholder, str>>;
type GetType = Pattern<SinglePlaceholder, str>;
type OwnedType = Pattern<SinglePlaceholder, String>;
}There was a problem hiding this comment.
There are still some errors, I will try to fix them in the morning.
| @@ -55,6 +55,7 @@ mod common; | |||
| mod double; | |||
| mod error; | |||
| mod frontend; | |||
| mod implementations; | |||
There was a problem hiding this comment.
Nit: make this
#[cfg(feature = "zerovec")]
mod zerovec;
utils/pattern/Cargo.toml
Outdated
| @@ -29,10 +29,10 @@ litemap = { workspace = true, default-features = false, optional = true } | |||
| serde = { workspace = true, features = ["derive", "alloc"], optional = true } | |||
| yoke = { workspace = true, features = ["derive"], optional = true } | |||
| zerofrom = { workspace = true, features = ["derive"], optional = true } | |||
| zerovec = { workspace = true, features = ["databake", "serde", "yoke"], optional = true } | |||
There was a problem hiding this comment.
Nit: Add an explicit zerovec = ["dep:zerovec"] feature
utils/pattern/src/implementations.rs
Outdated
|
|
||
| use crate::{Pattern, SinglePlaceholder}; | ||
|
|
||
| impl<'data> ZeroMapKV<'data> for Pattern<SinglePlaceholder, str> { |
There was a problem hiding this comment.
Nit: Add the impl for all pattern backends defined in this crate
|
This PR is a draft because it doesn't build. |
utils/pattern/src/implementations.rs
Outdated
| } | ||
|
|
||
| unsafe impl VarULE for Pattern<SinglePlaceholder, str> { | ||
| fn validate_byte_slice(_bytes: &[u8]) -> Result<(), zerovec::ZeroVecError> { |
There was a problem hiding this comment.
@sffc , is that the correct direction, or I can implement it in a different way ?
There was a problem hiding this comment.
Yeah, you'll need this impl. You should add a try_from_utf8 function on Pattern and call it from here.
There was a problem hiding this comment.
do you mean try_from_u8 ?
There was a problem hiding this comment.
I mean try_from_utf8 as agreed in #4931
utils/pattern/src/implementations.rs
Outdated
| let store = core::str::from_utf8_unchecked(bytes); | ||
| SinglePlaceholderPattern::from_borrowed_store_unchecked(store) |
There was a problem hiding this comment.
issue: justify these unsafe function calls
There was a problem hiding this comment.
wrong way around, you need to explain why each precondition of from_utf8_unchecked and of from_borrowed_store_unchecked holds.
There was a problem hiding this comment.
done, but I do not think I need to explain what each function do, the user can check this if they want.
| type Container = VarZeroVec<'a, Pattern<SinglePlaceholder, str>>; | ||
| type Slice = VarZeroSlice<Pattern<SinglePlaceholder, str>>; | ||
| type GetType = Pattern<SinglePlaceholder, str>; | ||
| type OwnedType = Box<Pattern<SinglePlaceholder, str>>; |
There was a problem hiding this comment.
should this be Pattern<SinglePlaceholder, String>?
There was a problem hiding this comment.
Yes if it's easy, but not super important
There was a problem hiding this comment.
it's just another transmute
There was a problem hiding this comment.
why it should be Pattern<SinglePlaceholder, String> , if we are using Pattern<SinglePlaceholder, str> in the datagen ?
There was a problem hiding this comment.
the implementation will be : #5084
Co-authored-by: Robert Bastian <[email protected]>
| } | ||
|
|
||
| #[cfg(feature = "alloc")] | ||
| unsafe impl VarULE for Pattern<SinglePlaceholder, str> { |
There was a problem hiding this comment.
Thought: this impl is narrower than required; better would be an impl where Store: VarULE
Non blocking because we can generalize later. But up to this point we've done a good job of keeping the impls general.
There was a problem hiding this comment.
I agree, but I am thinking also to add imple to Double Pattern or generalize this later.
utils/pattern/src/implementations.rs
Outdated
| .map_err(|_| ZeroVecError::VarZeroVecFormatError)?; | ||
| Ok(()) | ||
| } | ||
| /// SAFETY: The `bytes` slice must be validated by `Self::validate_byte_slice`. |
There was a problem hiding this comment.
the trait defines the safety invariants for this method, so you don't have to repeat it.
Co-authored-by: Robert Bastian <[email protected]>
Co-authored-by: Robert Bastian <[email protected]>
| type OwnedType = Box<Pattern<SinglePlaceholder, str>>; | ||
| } | ||
|
|
||
| unsafe impl VarULE for Pattern<SinglePlaceholder, str> { |
There was a problem hiding this comment.
The impl itself is unsafe, which means there are invariants on the trait that need to be followed: https://docs.rs/zerovec/latest/zerovec/ule/trait.VarULE.html#safety. Please check these and leave a comment
No description provided.