Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ZeroMapKV for Pattern<SinglePlaceholder, str> #5030

Merged
merged 33 commits into from
Jun 21, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
814c6f4
Implement ZeroMapKV for Pattern<SinglePlaceholder, Cow<'data, str>>
younies Jun 10, 2024
fe617b3
try to fix
younies Jun 10, 2024
bbc3d4f
add "yoke" feature
younies Jun 10, 2024
f7bd27b
Merge branch 'main' of github.com:unicode-org/icu4x into dev-develop-kv
younies Jun 11, 2024
faa9ea1
Merge branch 'dev-develop-kv' of github.com:younies/icu4x into dev-de…
younies Jun 11, 2024
d5ea128
fix the build
younies Jun 11, 2024
cbe0c8f
fix
younies Jun 11, 2024
31652bc
Merge branch 'main' into dev-develop-kv
younies Jun 14, 2024
8e5bf3c
Merge branch 'main' of github.com:unicode-org/icu4x into dev-develop-kv
younies Jun 18, 2024
14cd6d1
Merge branch 'dev-develop-kv' of github.com:younies/icu4x into dev-de…
younies Jun 18, 2024
f26a103
IMPLEMENT
younies Jun 18, 2024
d85f218
Merge branch 'dev-develop-kv' of github.com:younies/icu4x into dev-de…
younies Jun 18, 2024
642b59f
Merge branch 'main' of github.com:unicode-org/icu4x into dev-develop-kv
younies Jun 18, 2024
6780b95
fix
younies Jun 18, 2024
7ca8ea0
fix
younies Jun 18, 2024
e116cd4
add cfg
younies Jun 18, 2024
e40a784
sort
younies Jun 18, 2024
20e1a65
Merge branch 'main' of github.com:unicode-org/icu4x into dev-develop-kv
younies Jun 19, 2024
5b65ffc
fix CI
younies Jun 19, 2024
c48eee3
add Safety comment
younies Jun 19, 2024
0c841b9
Update utils/pattern/src/implementations.rs
younies Jun 19, 2024
80ca2df
Merge branch 'dev-develop-kv' of github.com:younies/icu4x into dev-de…
younies Jun 19, 2024
2d026d3
add alloc feature
younies Jun 19, 2024
2a9fb85
fix
younies Jun 19, 2024
b4d8161
fix CI
younies Jun 19, 2024
99af9a9
fix comment
younies Jun 19, 2024
ceb9991
Update utils/pattern/src/implementations.rs
younies Jun 20, 2024
1add0fe
Update utils/pattern/src/implementations.rs
younies Jun 20, 2024
0623700
Merge branch 'dev-develop-kv' of github.com:younies/icu4x into dev-de…
younies Jun 20, 2024
7b2f08b
remove unneeded comment
younies Jun 20, 2024
f3cb3cb
Merge branch 'main' of github.com:unicode-org/icu4x into dev-develop-kv
younies Jun 20, 2024
a4dc312
fix
younies Jun 20, 2024
1abc9fe
add comment
younies Jun 21, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion utils/pattern/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Add an explicit zerovec = ["dep:zerovec"] feature


[dev-dependencies]
zerofrom = { workspace = true, features = ["alloc"] }
zerovec = { workspace = true, features = ["databake", "serde"] }
rmp-serde = { workspace = true }
serde_json = { workspace = true }
postcard = { workspace = true, features = ["use-std"] }
Expand Down
26 changes: 26 additions & 0 deletions utils/pattern/src/implementations.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// This file is part of ICU4X. For terms of use, please see the file
// called LICENSE at the top level of the ICU4X source tree
// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ).

use zerovec::{maps::ZeroMapKV, ule::VarULE, VarZeroSlice, VarZeroVec, ZeroVecError};

use crate::{Pattern, SinglePlaceholder, SinglePlaceholderPattern};

impl<'a> ZeroMapKV<'a> for Pattern<SinglePlaceholder, str> {
type Container = VarZeroVec<'a, Pattern<SinglePlaceholder, str>>;
type Slice = VarZeroSlice<Pattern<SinglePlaceholder, str>>;
type GetType = Pattern<SinglePlaceholder, str>;
type OwnedType = Box<Pattern<SinglePlaceholder, str>>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be Pattern<SinglePlaceholder, String>?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes if it's easy, but not super important

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's just another transmute

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why it should be Pattern<SinglePlaceholder, String> , if we are using Pattern<SinglePlaceholder, str> in the datagen ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the implementation will be : #5084

}

unsafe impl VarULE for Pattern<SinglePlaceholder, str> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but I am thinking also to add imple to Double Pattern or generalize this later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

fn validate_byte_slice(bytes: &[u8]) -> Result<(), ZeroVecError> {
SinglePlaceholderPattern::try_from_bytes_store(bytes)
.map_err(|_| ZeroVecError::VarZeroVecFormatError)?;
Ok(())
}

unsafe fn from_byte_slice_unchecked(bytes: &[u8]) -> &Self {
&SinglePlaceholderPattern::try_from_bytes_store(bytes).unwrap()
}
}
1 change: 1 addition & 0 deletions utils/pattern/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ mod common;
mod double;
mod error;
mod frontend;
mod implementations;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: make this

#[cfg(feature = "zerovec")]
mod zerovec;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

mod multi_named;
#[cfg(feature = "alloc")]
mod parser;
Expand Down
Loading